From 43083423e42d94445896304f139ab5cc880572ce Mon Sep 17 00:00:00 2001 From: dfawley Date: Tue, 5 Dec 2017 10:04:04 -0800 Subject: [PATCH] Change parseTimeout to not handle non-second durations (#1706) --- service_config.go | 61 ++++++++++++++++++++++++++++------------ service_config_test.go | 63 ++++++++++++++++++++++++++++++++++++++++++ test/end2end_test.go | 12 ++++---- 3 files changed, 113 insertions(+), 23 deletions(-) diff --git a/service_config.go b/service_config.go index cde648334..53fa88f37 100644 --- a/service_config.go +++ b/service_config.go @@ -20,6 +20,9 @@ package grpc import ( "encoding/json" + "fmt" + "strconv" + "strings" "time" "google.golang.org/grpc/grpclog" @@ -70,12 +73,48 @@ type ServiceConfig struct { Methods map[string]MethodConfig } -func parseTimeout(t *string) (*time.Duration, error) { - if t == nil { +func parseDuration(s *string) (*time.Duration, error) { + if s == nil { return nil, nil } - d, err := time.ParseDuration(*t) - return &d, err + if !strings.HasSuffix(*s, "s") { + return nil, fmt.Errorf("malformed duration %q", *s) + } + ss := strings.SplitN((*s)[:len(*s)-1], ".", 3) + if len(ss) > 2 { + return nil, fmt.Errorf("malformed duration %q", *s) + } + // hasDigits is set if either the whole or fractional part of the number is + // present, since both are optional but one is required. + hasDigits := false + var d time.Duration + if len(ss[0]) > 0 { + i, err := strconv.ParseInt(ss[0], 10, 32) + if err != nil { + return nil, fmt.Errorf("malformed duration %q: %v", *s, err) + } + d = time.Duration(i) * time.Second + hasDigits = true + } + if len(ss) == 2 && len(ss[1]) > 0 { + if len(ss[1]) > 9 { + return nil, fmt.Errorf("malformed duration %q", *s) + } + f, err := strconv.ParseInt(ss[1], 10, 64) + if err != nil { + return nil, fmt.Errorf("malformed duration %q: %v", *s, err) + } + for i := 9; i > len(ss[1]); i-- { + f *= 10 + } + d += time.Duration(f) + hasDigits = true + } + if !hasDigits { + return nil, fmt.Errorf("malformed duration %q", *s) + } + + return &d, nil } type jsonName struct { @@ -128,7 +167,7 @@ func parseServiceConfig(js string) (ServiceConfig, error) { if m.Name == nil { continue } - d, err := parseTimeout(m.Timeout) + d, err := parseDuration(m.Timeout) if err != nil { grpclog.Warningf("grpc: parseServiceConfig error unmarshaling %s due to %v", js, err) return ServiceConfig{}, err @@ -182,18 +221,6 @@ func getMaxSize(mcMax, doptMax *int, defaultVal int) *int { return doptMax } -func newBool(b bool) *bool { - return &b -} - func newInt(b int) *int { return &b } - -func newDuration(b time.Duration) *time.Duration { - return &b -} - -func newString(b string) *string { - return &b -} diff --git a/service_config_test.go b/service_config_test.go index 7e985457e..8301a5061 100644 --- a/service_config_test.go +++ b/service_config_test.go @@ -19,6 +19,8 @@ package grpc import ( + "fmt" + "math" "reflect" "testing" "time" @@ -321,3 +323,64 @@ func TestPraseMsgSize(t *testing.T) { } } } + +func TestParseDuration(t *testing.T) { + testCases := []struct { + s *string + want *time.Duration + err bool + }{ + {s: nil, want: nil}, + {s: newString("1s"), want: newDuration(time.Second)}, + {s: newString("-1s"), want: newDuration(-time.Second)}, + {s: newString("1.1s"), want: newDuration(1100 * time.Millisecond)}, + {s: newString("1.s"), want: newDuration(time.Second)}, + {s: newString("1.0s"), want: newDuration(time.Second)}, + {s: newString(".002s"), want: newDuration(2 * time.Millisecond)}, + {s: newString(".002000s"), want: newDuration(2 * time.Millisecond)}, + {s: newString("0.003s"), want: newDuration(3 * time.Millisecond)}, + {s: newString("0.000004s"), want: newDuration(4 * time.Microsecond)}, + {s: newString("5000.000000009s"), want: newDuration(5000*time.Second + 9*time.Nanosecond)}, + {s: newString("4999.999999999s"), want: newDuration(5000*time.Second - time.Nanosecond)}, + {s: newString("1"), err: true}, + {s: newString("s"), err: true}, + {s: newString(".s"), err: true}, + {s: newString("1 s"), err: true}, + {s: newString(" 1s"), err: true}, + {s: newString("1ms"), err: true}, + {s: newString("1.1.1s"), err: true}, + {s: newString("Xs"), err: true}, + {s: newString("as"), err: true}, + {s: newString(".0000000001s"), err: true}, + {s: newString(fmt.Sprint(math.MaxInt32) + "s"), want: newDuration(math.MaxInt32 * time.Second)}, + {s: newString(fmt.Sprint(int64(math.MaxInt32)+1) + "s"), err: true}, + } + for _, tc := range testCases { + got, err := parseDuration(tc.s) + if tc.err != (err != nil) || + (got == nil) != (tc.want == nil) || + (got != nil && *got != *tc.want) { + wantErr := "" + if tc.err { + wantErr = "" + } + s := "" + if tc.s != nil { + s = `&"` + *tc.s + `"` + } + t.Errorf("parseDuration(%v) = %v, %v; want %v, %v", s, got, err, tc.want, wantErr) + } + } +} + +func newBool(b bool) *bool { + return &b +} + +func newDuration(b time.Duration) *time.Duration { + return &b +} + +func newString(b string) *string { + return &b +} diff --git a/test/end2end_test.go b/test/end2end_test.go index 8b0d1ac13..43a53a19b 100644 --- a/test/end2end_test.go +++ b/test/end2end_test.go @@ -1304,7 +1304,7 @@ func TestGetMethodConfig(t *testing.T) { } ], "waitForReady": true, - "timeout": "1ms" + "timeout": ".001s" }, { "name": [ @@ -1343,7 +1343,7 @@ func TestGetMethodConfig(t *testing.T) { } ], "waitForReady": true, - "timeout": "1ms" + "timeout": ".001s" }, { "name": [ @@ -1393,7 +1393,7 @@ func TestServiceConfigWaitForReady(t *testing.T) { } ], "waitForReady": false, - "timeout": "1ms" + "timeout": ".001s" } ] }`) @@ -1433,7 +1433,7 @@ func TestServiceConfigWaitForReady(t *testing.T) { } ], "waitForReady": true, - "timeout": "1ms" + "timeout": ".001s" } ] }`) @@ -1478,7 +1478,7 @@ func TestServiceConfigTimeout(t *testing.T) { } ], "waitForReady": true, - "timeout": "1h" + "timeout": "3600s" } ] }`) @@ -1523,7 +1523,7 @@ func TestServiceConfigTimeout(t *testing.T) { } ], "waitForReady": true, - "timeout": "1ns" + "timeout": ".000000001s" } ] }`)