-
Notifications
You must be signed in to change notification settings - Fork 595
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
Protect against ping periods that are out of range #488
Conversation
coordinate/client.go
Outdated
@@ -205,6 +207,11 @@ func (c *Client) Update(node string, other *Coordinate, rtt time.Duration) (*Coo | |||
return nil, err | |||
} | |||
|
|||
durSec := rtt.Seconds() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can do this as a time.Duration
(I'd put the constant right here, too):
const maxRTT = 10 * time.Second
if rtt < 0 || rtt > maxRTT {
return nil, fmt.Errorf("RTT %s out of range 0 to %s", rtt, maxRTT)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd also rail it way lower since 10 seconds is already several times around the Earth, so this should keep the coordinates better behaved in the face of weirdness (and it's not useful to model anything slower).
coordinate/client_test.go
Outdated
} | ||
|
||
dist_old := client.DistanceTo(other) | ||
// Update with a valid ping period, should have an affect on estimated rtt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"effect"
coordinate/client_test.go
Outdated
t.Fatal(err) | ||
} | ||
|
||
// Make sure the Euclidean part of our coordinate is what we expect. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This upper part doesn't seem necessary since we test the coordinate client elsewhere. I'd just try to update with a negative and a too large value and make sure you get the expected error back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Related to #487. |
I could reliably reproduce both extreme negative adjustment values and negative rtt estimates by feeding it time.maxvalue as the rtt value, which is a valid return values from the
time.Sub
method.This patch makes it so that we reject any updates that contain rtt durations that are not in a range from 1 to 32400 seconds (9 hours). Another option is to force updates > 9 hrs to be = 9hrs, but rejecting it seemed better to avoid corrupted data feeding into the algorithm.
The 32000 was something I found from some experimentation with Vivaldi starting with two sets of coordinates at the origin and trying to get them to drift to the point where adjustment becomes this large negative value. 9 hours also seems like a conservative but large enough upper bound on the previously unbounded rtt value.