Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib/connections: Validate device id before assuming success (fixes #5934) #5935

Merged

Conversation

@AudriusButkevicius
Copy link
Member

commented Aug 6, 2019

Duplicates some checks, but meh.

lib/connections/service.go Outdated Show resolved Hide resolved
lib/connections/service.go Outdated Show resolved Hide resolved
Vet
@calmh

calmh approved these changes Aug 7, 2019

Copy link
Member

left a comment

IMHO this check should really be in the (whateverDialer) Dial(deviceID ...) method. In principle one could assume that's what the first parameter (now unused) is for. And we could drop the "connected to self" bit altogether, as any connection to an unexpected ID will be dropped anyway.

(Approved, as I don't feel strongly enough about this to prevent merge, but I think it would be cleaner.)

// though, especially in the presence of NAT hairpinning, multiple
// clients between the same NAT gateway, and global discovery.
if remoteID == s.myID {
l.Infof("Connected to myself (%s) at %s - should not happen", remoteID, c)

This comment has been minimized.

Copy link
@calmh

calmh Aug 7, 2019

Member

Not related to this PR but I think we should drop this log message; it can and does happen, and the message just causes confusion.

This comment has been minimized.

Copy link
@AudriusButkevicius

AudriusButkevicius Aug 7, 2019

Author Member

It does help us to identify a certain type of problem, even if its not useful to the end user.

@AudriusButkevicius AudriusButkevicius merged commit 58ef536 into syncthing:master Aug 9, 2019

8 checks passed

Build (Linux, Cross) (Syncthing) TeamCity build finished
Details
Build (Mac) (Syncthing) TeamCity build finished
Details
Build (Source) (Syncthing) TeamCity build finished
Details
Build (Windows) (Syncthing) TeamCity build finished
Details
Check Correctness (Syncthing) TeamCity build finished
Details
Check Old Go Version (Syncthing) TeamCity build finished
Details
Coverage (Syncthing) TeamCity build finished
Details
GolangCI No issues found!
Details

@AudriusButkevicius AudriusButkevicius deleted the AudriusButkevicius:validate-before-ok branch Aug 9, 2019

@calmh calmh added this to the v1.2.2 milestone Aug 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.