From 99bd366db004f28050b19e3e867624a625b2c8d1 Mon Sep 17 00:00:00 2001 From: Preetha Appan Date: Thu, 19 Oct 2017 16:32:52 -0500 Subject: [PATCH 1/2] Protect against ping periods that are out of range --- coordinate/client.go | 7 +++++ coordinate/client_test.go | 55 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+) diff --git a/coordinate/client.go b/coordinate/client.go index 63f624141..b238dbf7f 100644 --- a/coordinate/client.go +++ b/coordinate/client.go @@ -8,6 +8,8 @@ import ( "time" ) +const MAX_RTT_SECONDS = 32400 //9 hours is the upper bound for ping period + // Client manages the estimated network coordinate for a given node, and adjusts // it as the node observes round trip times and estimated coordinates from other // nodes. The core algorithm is based on Vivaldi, see the documentation for Config @@ -205,6 +207,11 @@ func (c *Client) Update(node string, other *Coordinate, rtt time.Duration) (*Coo return nil, err } + durSec := rtt.Seconds() + if durSec <= 0 || durSec > MAX_RTT_SECONDS { + return nil, fmt.Errorf("Round trip time not in valid range, duration %v is not a positive value less than %v ", durSec, MAX_RTT_SECONDS) + } + rttSeconds := c.latencyFilter(node, rtt.Seconds()) c.updateVivaldi(other, rttSeconds) c.updateAdjustment(other, rttSeconds) diff --git a/coordinate/client_test.go b/coordinate/client_test.go index b84296d5e..dfe6ed937 100644 --- a/coordinate/client_test.go +++ b/coordinate/client_test.go @@ -1,6 +1,7 @@ package coordinate import ( + "fmt" "math" "reflect" "strings" @@ -64,6 +65,60 @@ func TestClient_Update(t *testing.T) { verifyEqualFloats(t, c.Vec[2], 99.0) } +func TestClient_InvalidInPingValues(t *testing.T) { + config := DefaultConfig() + config.Dimensionality = 3 + + client, err := NewClient(config) + if err != nil { + t.Fatal(err) + } + + // Make sure the Euclidean part of our coordinate is what we expect. + c := client.GetCoordinate() + verifyEqualVectors(t, c.Vec, []float64{0.0, 0.0, 0.0}) + + // Place another node + other := NewCoordinate(config) + other.Vec[2] = 0.001 + rtt := time.Duration(2.0 * other.Vec[2] * secondsToNanoseconds) + c, err = client.Update("node", other, rtt) + if err != nil { + t.Fatalf("err: %v", err) + } + + dist_old := client.DistanceTo(other) + // Update with a valid ping period, should have an affect on estimated rtt + ping := 20 + _, err = client.Update("node", other, time.Duration(ping*secondsToNanoseconds)) + if err != nil { + t.Fatalf("Unexpected error ", err) + } + + dist_new := client.DistanceTo(other) + if dist_new <= dist_old { + t.Fatalf("Invalid rtt estimate %v", dist_new) + } + + // Update with a series of invalid ping periods, should return an error and estimated rtt remains unchanged + pings := []int{1<<63 - 1, -35, -1 << 63, 35000} + dist_old = dist_new + + for _, ping := range pings { + expectedErr := fmt.Errorf("Round trip time not in valid range, duration %v is not a positive value less than %v", ping, MAX_RTT_SECONDS) + _, err = client.Update("node", other, time.Duration(ping*secondsToNanoseconds)) + if err == nil { + t.Fatalf("Unexpected error, wanted %v but got %v", expectedErr, err) + } + + dist_new = client.DistanceTo(other) + if dist_new != dist_old { + t.Fatalf("distance est", dist_new) + } + } + +} + func TestClient_DistanceTo(t *testing.T) { config := DefaultConfig() config.Dimensionality = 3 From 585c31cef5a890ffed05d58a27b6eb81090d2ef0 Mon Sep 17 00:00:00 2001 From: Preetha Appan Date: Thu, 19 Oct 2017 22:49:19 -0500 Subject: [PATCH 2/2] Removes some redundant test code, move const definition closer to where its used --- coordinate/client.go | 8 +++----- coordinate/client_test.go | 34 ++++++---------------------------- 2 files changed, 9 insertions(+), 33 deletions(-) diff --git a/coordinate/client.go b/coordinate/client.go index b238dbf7f..403ec7801 100644 --- a/coordinate/client.go +++ b/coordinate/client.go @@ -8,8 +8,6 @@ import ( "time" ) -const MAX_RTT_SECONDS = 32400 //9 hours is the upper bound for ping period - // Client manages the estimated network coordinate for a given node, and adjusts // it as the node observes round trip times and estimated coordinates from other // nodes. The core algorithm is based on Vivaldi, see the documentation for Config @@ -207,9 +205,9 @@ func (c *Client) Update(node string, other *Coordinate, rtt time.Duration) (*Coo return nil, err } - durSec := rtt.Seconds() - if durSec <= 0 || durSec > MAX_RTT_SECONDS { - return nil, fmt.Errorf("Round trip time not in valid range, duration %v is not a positive value less than %v ", durSec, MAX_RTT_SECONDS) + const maxRTT = 10 * time.Second + if rtt <= 0 || rtt > maxRTT { + return nil, fmt.Errorf("round trip time not in valid range, duration %v is not a positive value less than %v ", rtt, maxRTT) } rttSeconds := c.latencyFilter(node, rtt.Seconds()) diff --git a/coordinate/client_test.go b/coordinate/client_test.go index dfe6ed937..159ed05ea 100644 --- a/coordinate/client_test.go +++ b/coordinate/client_test.go @@ -74,46 +74,24 @@ func TestClient_InvalidInPingValues(t *testing.T) { t.Fatal(err) } - // Make sure the Euclidean part of our coordinate is what we expect. - c := client.GetCoordinate() - verifyEqualVectors(t, c.Vec, []float64{0.0, 0.0, 0.0}) - // Place another node other := NewCoordinate(config) other.Vec[2] = 0.001 - rtt := time.Duration(2.0 * other.Vec[2] * secondsToNanoseconds) - c, err = client.Update("node", other, rtt) - if err != nil { - t.Fatalf("err: %v", err) - } - - dist_old := client.DistanceTo(other) - // Update with a valid ping period, should have an affect on estimated rtt - ping := 20 - _, err = client.Update("node", other, time.Duration(ping*secondsToNanoseconds)) - if err != nil { - t.Fatalf("Unexpected error ", err) - } - - dist_new := client.DistanceTo(other) - if dist_new <= dist_old { - t.Fatalf("Invalid rtt estimate %v", dist_new) - } + dist := client.DistanceTo(other) // Update with a series of invalid ping periods, should return an error and estimated rtt remains unchanged - pings := []int{1<<63 - 1, -35, -1 << 63, 35000} - dist_old = dist_new + pings := []int{1<<63 - 1, -35, 11} for _, ping := range pings { - expectedErr := fmt.Errorf("Round trip time not in valid range, duration %v is not a positive value less than %v", ping, MAX_RTT_SECONDS) + expectedErr := fmt.Errorf("round trip time not in valid range, duration %v is not a positive value less than %v", ping, 10*time.Second) _, err = client.Update("node", other, time.Duration(ping*secondsToNanoseconds)) if err == nil { t.Fatalf("Unexpected error, wanted %v but got %v", expectedErr, err) } - dist_new = client.DistanceTo(other) - if dist_new != dist_old { - t.Fatalf("distance est", dist_new) + dist_new := client.DistanceTo(other) + if dist_new != dist { + t.Fatalf("distance estimate %v not equal to %v", dist_new, dist) } }