From 4846b58453b3708320bdb524f25cc5a1d9cda4d4 Mon Sep 17 00:00:00 2001 From: Herbie Ong Date: Wed, 14 Oct 2020 23:18:29 -0700 Subject: [PATCH] jsonpb: Fix marshaling of Duration (#1221) Negative nanosecond should not have negative sign after decimal point. Add check for max and min seconds. Fixes #1219. --- jsonpb/encode.go | 11 ++++++++--- jsonpb/json_test.go | 35 ++++++++++++++++++++++------------- 2 files changed, 30 insertions(+), 16 deletions(-) diff --git a/jsonpb/encode.go b/jsonpb/encode.go index 7633019f72..685c80a62b 100644 --- a/jsonpb/encode.go +++ b/jsonpb/encode.go @@ -166,20 +166,25 @@ func (w *jsonWriter) marshalMessage(m protoreflect.Message, indent, typeURL stri fd := fds.ByNumber(1) return w.marshalValue(fd, m.Get(fd), indent) case "Duration": + const maxSecondsInDuration = 315576000000 // "Generated output always contains 0, 3, 6, or 9 fractional digits, // depending on required precision." s := m.Get(fds.ByNumber(1)).Int() ns := m.Get(fds.ByNumber(2)).Int() + if s < -maxSecondsInDuration || s > maxSecondsInDuration { + return fmt.Errorf("seconds out of range %v", s) + } if ns <= -secondInNanos || ns >= secondInNanos { return fmt.Errorf("ns out of range (%v, %v)", -secondInNanos, secondInNanos) } if (s > 0 && ns < 0) || (s < 0 && ns > 0) { return errors.New("signs of seconds and nanos do not match") } - if s < 0 { - ns = -ns + var sign string + if s < 0 || ns < 0 { + sign, s, ns = "-", -1*s, -1*ns } - x := fmt.Sprintf("%d.%09d", s, ns) + x := fmt.Sprintf("%s%d.%09d", sign, s, ns) x = strings.TrimSuffix(x, "000") x = strings.TrimSuffix(x, "000") x = strings.TrimSuffix(x, ".000") diff --git a/jsonpb/json_test.go b/jsonpb/json_test.go index 485b29279e..0ef23f2d30 100644 --- a/jsonpb/json_test.go +++ b/jsonpb/json_test.go @@ -448,10 +448,17 @@ var marshalingTests = []struct { {"Any with message and indent", marshalerAllOptions, anySimple, anySimplePrettyJSON}, {"Any with WKT", marshaler, anyWellKnown, anyWellKnownJSON}, {"Any with WKT and indent", marshalerAllOptions, anyWellKnown, anyWellKnownPrettyJSON}, - {"Duration", marshaler, &pb2.KnownTypes{Dur: &durpb.Duration{Seconds: 3}}, `{"dur":"3s"}`}, - {"Duration", marshaler, &pb2.KnownTypes{Dur: &durpb.Duration{Seconds: 3, Nanos: 1e6}}, `{"dur":"3.001s"}`}, - {"Duration beyond float64 precision", marshaler, &pb2.KnownTypes{Dur: &durpb.Duration{Seconds: 100000000, Nanos: 1}}, `{"dur":"100000000.000000001s"}`}, - {"negative Duration", marshaler, &pb2.KnownTypes{Dur: &durpb.Duration{Seconds: -123, Nanos: -456}}, `{"dur":"-123.000000456s"}`}, + {"Duration empty", marshaler, &durpb.Duration{}, `"0s"`}, + {"Duration with secs", marshaler, &durpb.Duration{Seconds: 3}, `"3s"`}, + {"Duration with -secs", marshaler, &durpb.Duration{Seconds: -3}, `"-3s"`}, + {"Duration with nanos", marshaler, &durpb.Duration{Nanos: 1e6}, `"0.001s"`}, + {"Duration with -nanos", marshaler, &durpb.Duration{Nanos: -1e6}, `"-0.001s"`}, + {"Duration with large secs", marshaler, &durpb.Duration{Seconds: 1e10, Nanos: 1}, `"10000000000.000000001s"`}, + {"Duration with 6-digit nanos", marshaler, &durpb.Duration{Nanos: 1e4}, `"0.000010s"`}, + {"Duration with 3-digit nanos", marshaler, &durpb.Duration{Nanos: 1e6}, `"0.001s"`}, + {"Duration with -secs -nanos", marshaler, &durpb.Duration{Seconds: -123, Nanos: -450}, `"-123.000000450s"`}, + {"Duration max value", marshaler, &durpb.Duration{Seconds: 315576000000, Nanos: 999999999}, `"315576000000.999999999s"`}, + {"Duration min value", marshaler, &durpb.Duration{Seconds: -315576000000, Nanos: -999999999}, `"-315576000000.999999999s"`}, {"Struct", marshaler, &pb2.KnownTypes{St: &stpb.Struct{ Fields: map[string]*stpb.Value{ "one": {Kind: &stpb.Value_StringValue{"loneliest number"}}, @@ -524,15 +531,17 @@ func TestMarshalIllegalTime(t *testing.T) { pb proto.Message fail bool }{ - {&pb2.KnownTypes{Dur: &durpb.Duration{Seconds: 1, Nanos: 0}}, false}, - {&pb2.KnownTypes{Dur: &durpb.Duration{Seconds: -1, Nanos: 0}}, false}, - {&pb2.KnownTypes{Dur: &durpb.Duration{Seconds: 1, Nanos: -1}}, true}, - {&pb2.KnownTypes{Dur: &durpb.Duration{Seconds: -1, Nanos: 1}}, true}, - {&pb2.KnownTypes{Dur: &durpb.Duration{Seconds: 1, Nanos: 1000000000}}, true}, - {&pb2.KnownTypes{Dur: &durpb.Duration{Seconds: -1, Nanos: -1000000000}}, true}, - {&pb2.KnownTypes{Ts: &tspb.Timestamp{Seconds: 1, Nanos: 1}}, false}, - {&pb2.KnownTypes{Ts: &tspb.Timestamp{Seconds: 1, Nanos: -1}}, true}, - {&pb2.KnownTypes{Ts: &tspb.Timestamp{Seconds: 1, Nanos: 1000000000}}, true}, + {&durpb.Duration{Seconds: 1, Nanos: 0}, false}, + {&durpb.Duration{Seconds: -1, Nanos: 0}, false}, + {&durpb.Duration{Seconds: 1, Nanos: -1}, true}, + {&durpb.Duration{Seconds: -1, Nanos: 1}, true}, + {&durpb.Duration{Seconds: 315576000001}, true}, + {&durpb.Duration{Seconds: -315576000001}, true}, + {&durpb.Duration{Seconds: 1, Nanos: 1000000000}, true}, + {&durpb.Duration{Seconds: -1, Nanos: -1000000000}, true}, + {&tspb.Timestamp{Seconds: 1, Nanos: 1}, false}, + {&tspb.Timestamp{Seconds: 1, Nanos: -1}, true}, + {&tspb.Timestamp{Seconds: 1, Nanos: 1000000000}, true}, } for _, tt := range tests { _, err := marshaler.MarshalToString(tt.pb)