8000 Reject empty peers by brb · Pull Request #2501 · weaveworks/weave · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Jun 20, 2024. It is now read-only.

Reject empty peers #2501

Merged
merged 1 commit into from
Sep 21, 2016
Merged

Reject empty peers #2501

merged 1 commit into from
Sep 21, 2016

Conversation

brb
Copy link
Contributor
@brb brb commented Sep 5, 2016

Fix #2495

@brb brb added this to the 1.6.2 milestone Sep 5, 2016
@brb brb changed the title Filter empty peers WIP: Filter empty peers Sep 5, 2016
@brb brb changed the title WIP: Filter empty peers [WIP] Filter empty peers Sep 5, 2016
@brb
Copy link
Contributor Author
brb commented Sep 5, 2016

Just realized that the same check is needed for weave connect, so maybe adding it to the mesh library makes more sense?

rade
rade reviewed Sep 5, 2016
View reviewed changes
prog/weaver/main.go
@@ -575,3 +575,15 @@ func checkFatal(e error) {
Log.Fatal(e)
}
}

func filterEmpty(list []string) []string {
filtered := list[:0]

This comment was marked as abuse.

Sign in to view

@rade
Copy link
Member
rade commented Sep 5, 2016

so maybe adding it to the mesh library makes more sense?

This is a user input sanitisation issue s doesn't really belong into a library; the mesh library should be able to assume it operates on clean data.

@rade
Copy link
Member
rade commented Sep 5, 2016

I am not even convinced we should be doing any filtering here.

Where does the panic in #2495 originate?

@rade
Copy link
Member
rade commented Sep 5, 2016

Seems to me that the empty address should be rejected in connection_maker.InitiateConnections, just like it rejects other invalid addresses.

However, first I'd like to understand where/why the panic arises.

@brb
Copy link
Contributor Author
brb commented Sep 5, 2016 •
edited
Loading

Because https://github.com/weaveworks/mesh/blob/master/connection_maker.go#L89 does not check whether a host has been supplied and later on, addr is set to ":0" which evaluates to 127.0.0.1:$RANDOM_PORT.

@rade
Copy link
Member
rade commented Sep 5, 2016

but according to the stack trace you posted, it fails in connectionTerminated.

@rade rade assigned brb Sep 5, 2016
@brb
Copy link
Contributor Author
brb commented Sep 6, 2016 
connectionMaker.InitiateConnections(...)
   -> addrs[""] = ":0"
   -> request actor loop
        -> cm.directPeers[""] = ":0"
        -> connectionMaker.checkStateAndAttempConnections(...)
             -> cm.targets[":6783"] = &target{...}
             -> try to establish a connection (to itself)
                  -> connection fails => LocalConnection.teardown(...)
                       -> connectionMaker.connectionTerminated(...)
                            -> cm.targets["$IP:$PORT"] returns nil which makes it to panic

@rade
Copy link
Member
rade commented Sep 6, 2016

Right. So still it ...

Seems to me that the empty address should be rejected in connection_maker.InitiateConnections, just like it rejects other invalid addresses.

@brb
Copy link
Contributor Author
brb commented Sep 6, 2016

Yes.

@brb
Copy link
Contributor Author
brb commented Sep 13, 2016

weaveworks/mesh#58

@brb brb closed this Sep 13, 2016
@brb brb reopened this Sep 13, 2016
@brb
Copy link
Contributor Author
brb commented Sep 13, 2016

Need to update mesh submodule after weaveworks/mesh#58 gets merged.

@brb brb changed the title [WIP] Filter empty peers Reject empty peers Sep 14, 2016
@brb brb force-pushed the issues/2495-filter-empty-peers branch from 0c91d93 to 5caeb38 Compare September 14, 2016 16:15
@awh
Copy link
Contributor
awh commented Sep 20, 2016

@brb I have merged the mesh PR - update at will.

@brb brb force-pushed the issues/2495-filter-empty-peers branch from 5caeb38 to 6df2782 Compare September 20, 2016 16:03
@brb brb assigned awh and unassigned brb Sep 20, 2016
@awh awh merged commit cd4cbed into 1.6 Sep 21, 2016
@awh awh deleted the issues/2495-filter-empty-peers branch September 21, 2016 10:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@awh awh

Labels
None yet
Projects
None yet
Milestone
1.6.2
Development

Successfully merging this pull request may close these issues.
3 participants
@brb @rade @awh
0