From 4a42e5dc51331f00c925470c237a880c98688420 Mon Sep 17 00:00:00 2001 From: Evan Jones Date: Tue, 6 May 2025 11:43:24 -0400 Subject: [PATCH 1/2] internal/transport.decodeTimeout: ParseUint to reject negative Negative timeouts should be forbidden. The previous implementation called ParseInt and allowed negative timeouts. Change it to call ParseUint to reject these values. This is also slightly faster, since ParseInt eventually calles ParseUint. Extend the test to cover more cases. --- internal/transport/http_util.go | 4 ++-- internal/transport/http_util_test.go | 22 ++++++++++++++++++++-- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/internal/transport/http_util.go b/internal/transport/http_util.go index 3613d7b64817..0d3ddcf16252 100644 --- a/internal/transport/http_util.go +++ b/internal/transport/http_util.go @@ -196,11 +196,11 @@ func decodeTimeout(s string) (time.Duration, error) { if !ok { return 0, fmt.Errorf("transport: timeout unit is not recognized: %q", s) } - t, err := strconv.ParseInt(s[:size-1], 10, 64) + t, err := strconv.ParseUint(s[:size-1], 10, 64) if err != nil { return 0, err } - const maxHours = math.MaxInt64 / int64(time.Hour) + const maxHours = math.MaxInt64 / uint64(time.Hour) if d == time.Hour && t > maxHours { // This timeout would overflow math.MaxInt64; clamp it. return time.Duration(math.MaxInt64), nil diff --git a/internal/transport/http_util_test.go b/internal/transport/http_util_test.go index 5a259d43cdc2..743c218e2e73 100644 --- a/internal/transport/http_util_test.go +++ b/internal/transport/http_util_test.go @@ -22,13 +22,14 @@ import ( "errors" "fmt" "io" + "math" "net" "reflect" "testing" "time" ) -func (s) TestTimeoutDecode(t *testing.T) { +func (s) TestDecodeTimeout(t *testing.T) { for _, test := range []struct { // input s string @@ -36,14 +37,31 @@ func (s) TestTimeoutDecode(t *testing.T) { d time.Duration err error }{ + {"0S", 0, nil}, + {"00000000S", 0, nil}, + {"00000001n", time.Nanosecond, nil}, + {"10u", time.Microsecond * 10, nil}, + {"00000010m", time.Millisecond * 10, nil}, {"1234S", time.Second * 1234, nil}, + {"00000001M", time.Minute, nil}, + {"09999999S", time.Second * 9999999, nil}, + {"99999999S", time.Second * 99999999, nil}, + {"99999999M", time.Minute * 99999999, nil}, + {"2562047H", time.Hour * 2562047, nil}, + {"2562048H", time.Duration(math.MaxInt64), nil}, + {"99999999H", time.Duration(math.MaxInt64), nil}, + {"-1S", 0, fmt.Errorf("strconv.ParseUint: parsing %q: invalid syntax", "-1")}, {"1234x", 0, fmt.Errorf("transport: timeout unit is not recognized: %q", "1234x")}, + {"1234s", 0, fmt.Errorf("transport: timeout unit is not recognized: %q", "1234s")}, + {"1234", 0, fmt.Errorf("transport: timeout unit is not recognized: %q", "1234")}, {"1", 0, fmt.Errorf("transport: timeout string is too short: %q", "1")}, {"", 0, fmt.Errorf("transport: timeout string is too short: %q", "")}, + {"9a1S", 0, fmt.Errorf("strconv.ParseUint: parsing %q: invalid syntax", "9a1")}, + {"000000000S", 0, fmt.Errorf("transport: timeout string is too long: %q", "000000000S")}, } { d, err := decodeTimeout(test.s) if d != test.d || fmt.Sprint(err) != fmt.Sprint(test.err) { - t.Fatalf("timeoutDecode(%q) = %d, %v, want %d, %v", test.s, int64(d), err, int64(test.d), test.err) + t.Errorf("timeoutDecode(%q) = %d, %v, want %d, %v", test.s, int64(d), err, int64(test.d), test.err) } } } From 98de62f81a3cb41d5e3ff22dc1063843845b0dbb Mon Sep 17 00:00:00 2001 From: Evan Jones Date: Wed, 7 May 2025 14:27:47 -0400 Subject: [PATCH 2/2] code review improvements * forbid timeout == 0 * change test to check for err != nil and not compare messages --- internal/transport/http_util.go | 3 ++ internal/transport/http_util_test.go | 53 +++++++++++++++------------- 2 files changed, 31 insertions(+), 25 deletions(-) diff --git a/internal/transport/http_util.go b/internal/transport/http_util.go index 0d3ddcf16252..968d556a1f1b 100644 --- a/internal/transport/http_util.go +++ b/internal/transport/http_util.go @@ -200,6 +200,9 @@ func decodeTimeout(s string) (time.Duration, error) { if err != nil { return 0, err } + if t == 0 { + return 0, fmt.Errorf("transport: timeout must be positive: %q", s) + } const maxHours = math.MaxInt64 / uint64(time.Hour) if d == time.Hour && t > maxHours { // This timeout would overflow math.MaxInt64; clamp it. diff --git a/internal/transport/http_util_test.go b/internal/transport/http_util_test.go index 743c218e2e73..546e8e7f39dc 100644 --- a/internal/transport/http_util_test.go +++ b/internal/transport/http_util_test.go @@ -34,34 +34,37 @@ func (s) TestDecodeTimeout(t *testing.T) { // input s string // output - d time.Duration - err error + d time.Duration + wantErr bool }{ - {"0S", 0, nil}, - {"00000000S", 0, nil}, - {"00000001n", time.Nanosecond, nil}, - {"10u", time.Microsecond * 10, nil}, - {"00000010m", time.Millisecond * 10, nil}, - {"1234S", time.Second * 1234, nil}, - {"00000001M", time.Minute, nil}, - {"09999999S", time.Second * 9999999, nil}, - {"99999999S", time.Second * 99999999, nil}, - {"99999999M", time.Minute * 99999999, nil}, - {"2562047H", time.Hour * 2562047, nil}, - {"2562048H", time.Duration(math.MaxInt64), nil}, - {"99999999H", time.Duration(math.MaxInt64), nil}, - {"-1S", 0, fmt.Errorf("strconv.ParseUint: parsing %q: invalid syntax", "-1")}, - {"1234x", 0, fmt.Errorf("transport: timeout unit is not recognized: %q", "1234x")}, - {"1234s", 0, fmt.Errorf("transport: timeout unit is not recognized: %q", "1234s")}, - {"1234", 0, fmt.Errorf("transport: timeout unit is not recognized: %q", "1234")}, - {"1", 0, fmt.Errorf("transport: timeout string is too short: %q", "1")}, - {"", 0, fmt.Errorf("transport: timeout string is too short: %q", "")}, - {"9a1S", 0, fmt.Errorf("strconv.ParseUint: parsing %q: invalid syntax", "9a1")}, - {"000000000S", 0, fmt.Errorf("transport: timeout string is too long: %q", "000000000S")}, + + {"00000001n", time.Nanosecond, false}, + {"10u", time.Microsecond * 10, false}, + {"00000010m", time.Millisecond * 10, false}, + {"1234S", time.Second * 1234, false}, + {"00000001M", time.Minute, false}, + {"09999999S", time.Second * 9999999, false}, + {"99999999S", time.Second * 99999999, false}, + {"99999999M", time.Minute * 99999999, false}, + {"2562047H", time.Hour * 2562047, false}, + {"2562048H", time.Duration(math.MaxInt64), false}, + {"99999999H", time.Duration(math.MaxInt64), false}, + {"-1S", 0, true}, + {"1234x", 0, true}, + {"1234s", 0, true}, + {"1234", 0, true}, + {"1", 0, true}, + {"", 0, true}, + {"9a1S", 0, true}, + {"0S", 0, true}, // PROTOCOL-HTTP2.md requires positive integers + {"00000000S", 0, true}, + {"000000000S", 0, true}, } { d, err := decodeTimeout(test.s) - if d != test.d || fmt.Sprint(err) != fmt.Sprint(test.err) { - t.Errorf("timeoutDecode(%q) = %d, %v, want %d, %v", test.s, int64(d), err, int64(test.d), test.err) + gotErr := err != nil + if d != test.d || gotErr != test.wantErr { + t.Errorf("timeoutDecode(%q) = %d, %v, want %d, wantErr=%v", + test.s, int64(d), err, int64(test.d), test.wantErr) } } }