From 0a2722adfd1186deee784a5dfe7890025b5fb0c2 Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Mon, 22 Jan 2018 09:36:21 -0800 Subject: [PATCH 1/4] fix handling of illegal and negative durations --- jsonpb/jsonpb.go | 9 +++++++-- jsonpb/jsonpb_test.go | 24 ++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/jsonpb/jsonpb.go b/jsonpb/jsonpb.go index 601fb3164a..f5819e04e5 100644 --- a/jsonpb/jsonpb.go +++ b/jsonpb/jsonpb.go @@ -201,8 +201,13 @@ func (m *Marshaler) marshalObject(out *errWriter, v proto.Message, indent, typeU // "Generated output always contains 3, 6, or 9 fractional digits, // depending on required precision." s, ns := s.Field(0).Int(), s.Field(1).Int() - d := time.Duration(s)*time.Second + time.Duration(ns)*time.Nanosecond - x := fmt.Sprintf("%.9f", d.Seconds()) + 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 + } + x := fmt.Sprintf("%d.%09d", s, ns) x = strings.TrimSuffix(x, "000") x = strings.TrimSuffix(x, "000") out.write(`"`) diff --git a/jsonpb/jsonpb_test.go b/jsonpb/jsonpb_test.go index d3ea5b2e63..2920627f29 100644 --- a/jsonpb/jsonpb_test.go +++ b/jsonpb/jsonpb_test.go @@ -407,6 +407,8 @@ var marshalingTests = []struct { {"Any with WKT", marshaler, anyWellKnown, anyWellKnownJSON}, {"Any with WKT and indent", marshalerAllOptions, anyWellKnown, anyWellKnownPrettyJSON}, {"Duration", marshaler, &pb.KnownTypes{Dur: &durpb.Duration{Seconds: 3}}, `{"dur":"3.000s"}`}, + {"Duration beyond float64 precision", marshaler, &pb.KnownTypes{Dur: &durpb.Duration{Seconds: 100000000, Nanos: 1}}, `{"dur":"100000000.000000001s"}`}, + {"negative Duration", marshaler, &pb.KnownTypes{Dur: &durpb.Duration{Seconds: -123, Nanos: -456}}, `{"dur":"-123.000000456s"}`}, {"Struct", marshaler, &pb.KnownTypes{St: &stpb.Struct{ Fields: map[string]*stpb.Value{ "one": {Kind: &stpb.Value_StringValue{"loneliest number"}}, @@ -473,6 +475,28 @@ func TestMarshalingNil(t *testing.T) { } } +func TestMarshalIllegalDuration(t *testing.T) { + tests := []struct { + pb proto.Message + ok bool + }{ + {&pb.KnownTypes{Dur: &durpb.Duration{Seconds: 1, Nanos: 0}}, true}, + {&pb.KnownTypes{Dur: &durpb.Duration{Seconds: -1, Nanos: 0}}, true}, + {&pb.KnownTypes{Dur: &durpb.Duration{Seconds: 1, Nanos: -1}}, false}, + {&pb.KnownTypes{Dur: &durpb.Duration{Seconds: -1, Nanos: 1}}, false}, + } + const errWant = "signs of seconds and nanos do not match" + for _, tt := range tests { + got, err := marshaler.MarshalToString(tt.pb) + if !tt.ok && (err == nil || !strings.Contains(err.Error(), errWant)) { + t.Errorf("marshaler.MarshalToString(%v) = %v, %v; want _, ", tt.pb, got, err, errWant) + } + if tt.ok && err != nil { + t.Errorf("marshaler.MarshalToString(%v) = %v, %v; want _, ", tt.pb, got, err) + } + } +} + func TestMarshalJSONPBMarshaler(t *testing.T) { rawJson := `{ "foo": "bar", "baz": [0, 1, 2, 3] }` msg := dynamicMessage{rawJson: rawJson} From e35b0e17fe583674df00457da38af6a8aa9a775f Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Mon, 22 Jan 2018 10:28:14 -0800 Subject: [PATCH 2/4] fix existing test Errorf problem --- jsonpb/jsonpb_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jsonpb/jsonpb_test.go b/jsonpb/jsonpb_test.go index 2920627f29..c54e12f4b4 100644 --- a/jsonpb/jsonpb_test.go +++ b/jsonpb/jsonpb_test.go @@ -944,7 +944,7 @@ func TestUnmarshalAnyJSONPBUnmarshaler(t *testing.T) { } if !proto.Equal(&got, &want) { - t.Errorf("message contents not set correctly after unmarshalling JSON: got %s, wanted %s", got, want) + t.Errorf("message contents not set correctly after unmarshalling JSON: got %v, wanted %v", got, want) } } From 7437c35f33b01935c8d96240a10862ee06af5939 Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Mon, 22 Jan 2018 14:32:12 -0800 Subject: [PATCH 3/4] add validation for ns range --- jsonpb/jsonpb.go | 8 ++++++++ jsonpb/jsonpb_test.go | 32 +++++++++++++++++++------------- 2 files changed, 27 insertions(+), 13 deletions(-) diff --git a/jsonpb/jsonpb.go b/jsonpb/jsonpb.go index f5819e04e5..e888e1f35a 100644 --- a/jsonpb/jsonpb.go +++ b/jsonpb/jsonpb.go @@ -56,6 +56,8 @@ import ( stpb "github.com/golang/protobuf/ptypes/struct" ) +const secondInNanos = int64(time.Second / time.Nanosecond) + // Marshaler is a configurable object for converting between // protocol buffer objects and a JSON representation for them. type Marshaler struct { @@ -201,6 +203,9 @@ func (m *Marshaler) marshalObject(out *errWriter, v proto.Message, indent, typeU // "Generated output always contains 3, 6, or 9 fractional digits, // depending on required precision." s, ns := s.Field(0).Int(), s.Field(1).Int() + 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") } @@ -222,6 +227,9 @@ func (m *Marshaler) marshalObject(out *errWriter, v proto.Message, indent, typeU // "RFC 3339, where generated output will always be Z-normalized // and uses 3, 6 or 9 fractional digits." s, ns := s.Field(0).Int(), s.Field(1).Int() + if ns < 0 || ns >= secondInNanos { + return fmt.Errorf("ns out of range [0, %v)", secondInNanos) + } t := time.Unix(s, ns).UTC() // time.RFC3339Nano isn't exactly right (we need to get 3/6/9 fractional digits). x := t.Format("2006-01-02T15:04:05.000000000") diff --git a/jsonpb/jsonpb_test.go b/jsonpb/jsonpb_test.go index c54e12f4b4..e6d2206846 100644 --- a/jsonpb/jsonpb_test.go +++ b/jsonpb/jsonpb_test.go @@ -34,6 +34,7 @@ package jsonpb import ( "bytes" "encoding/json" + "errors" "io" "math" "reflect" @@ -475,24 +476,29 @@ func TestMarshalingNil(t *testing.T) { } } -func TestMarshalIllegalDuration(t *testing.T) { +func TestMarshalIllegalTime(t *testing.T) { tests := []struct { - pb proto.Message - ok bool + pb proto.Message + errWant error }{ - {&pb.KnownTypes{Dur: &durpb.Duration{Seconds: 1, Nanos: 0}}, true}, - {&pb.KnownTypes{Dur: &durpb.Duration{Seconds: -1, Nanos: 0}}, true}, - {&pb.KnownTypes{Dur: &durpb.Duration{Seconds: 1, Nanos: -1}}, false}, - {&pb.KnownTypes{Dur: &durpb.Duration{Seconds: -1, Nanos: 1}}, false}, + {&pb.KnownTypes{Dur: &durpb.Duration{Seconds: 1, Nanos: 0}}, nil}, + {&pb.KnownTypes{Dur: &durpb.Duration{Seconds: -1, Nanos: 0}}, nil}, + {&pb.KnownTypes{Dur: &durpb.Duration{Seconds: 1, Nanos: -1}}, errors.New("signs of seconds and nanos do not match")}, + {&pb.KnownTypes{Dur: &durpb.Duration{Seconds: -1, Nanos: 1}}, errors.New("signs of seconds and nanos do not match")}, + {&pb.KnownTypes{Dur: &durpb.Duration{Seconds: 1, Nanos: 1000000000}}, errors.New("ns out of range")}, + {&pb.KnownTypes{Dur: &durpb.Duration{Seconds: -1, Nanos: -1000000000}}, errors.New("ns out of range")}, + {&pb.KnownTypes{Ts: &tspb.Timestamp{Seconds: 1, Nanos: 1}}, nil}, + {&pb.KnownTypes{Ts: &tspb.Timestamp{Seconds: 1, Nanos: -1}}, errors.New("ns out of range")}, + {&pb.KnownTypes{Ts: &tspb.Timestamp{Seconds: 1, Nanos: 1000000000}}, errors.New("ns out of range")}, } - const errWant = "signs of seconds and nanos do not match" for _, tt := range tests { - got, err := marshaler.MarshalToString(tt.pb) - if !tt.ok && (err == nil || !strings.Contains(err.Error(), errWant)) { - t.Errorf("marshaler.MarshalToString(%v) = %v, %v; want _, ", tt.pb, got, err, errWant) + _, err := marshaler.MarshalToString(tt.pb) + if (err == nil) != (tt.errWant == nil) { + t.Errorf("marshaler.MarshalToString(%v) = _, %v; want _, %v", tt.pb, err, tt.errWant) + continue } - if tt.ok && err != nil { - t.Errorf("marshaler.MarshalToString(%v) = %v, %v; want _, ", tt.pb, got, err) + if err != nil && !strings.Contains(err.Error(), tt.errWant.Error()) { + t.Errorf("marshaler.MarshalToString(%v) = _, %v; want _, %v", tt.pb, err, tt.errWant) } } } From 96b08e6e91f832c5dc0140d61d0986f014ea7a97 Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Mon, 22 Jan 2018 16:20:48 -0800 Subject: [PATCH 4/4] remove error string testing --- jsonpb/jsonpb_test.go | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/jsonpb/jsonpb_test.go b/jsonpb/jsonpb_test.go index e6d2206846..94b4c598d7 100644 --- a/jsonpb/jsonpb_test.go +++ b/jsonpb/jsonpb_test.go @@ -34,7 +34,6 @@ package jsonpb import ( "bytes" "encoding/json" - "errors" "io" "math" "reflect" @@ -478,27 +477,26 @@ func TestMarshalingNil(t *testing.T) { func TestMarshalIllegalTime(t *testing.T) { tests := []struct { - pb proto.Message - errWant error + pb proto.Message + fail bool }{ - {&pb.KnownTypes{Dur: &durpb.Duration{Seconds: 1, Nanos: 0}}, nil}, - {&pb.KnownTypes{Dur: &durpb.Duration{Seconds: -1, Nanos: 0}}, nil}, - {&pb.KnownTypes{Dur: &durpb.Duration{Seconds: 1, Nanos: -1}}, errors.New("signs of seconds and nanos do not match")}, - {&pb.KnownTypes{Dur: &durpb.Duration{Seconds: -1, Nanos: 1}}, errors.New("signs of seconds and nanos do not match")}, - {&pb.KnownTypes{Dur: &durpb.Duration{Seconds: 1, Nanos: 1000000000}}, errors.New("ns out of range")}, - {&pb.KnownTypes{Dur: &durpb.Duration{Seconds: -1, Nanos: -1000000000}}, errors.New("ns out of range")}, - {&pb.KnownTypes{Ts: &tspb.Timestamp{Seconds: 1, Nanos: 1}}, nil}, - {&pb.KnownTypes{Ts: &tspb.Timestamp{Seconds: 1, Nanos: -1}}, errors.New("ns out of range")}, - {&pb.KnownTypes{Ts: &tspb.Timestamp{Seconds: 1, Nanos: 1000000000}}, errors.New("ns out of range")}, + {&pb.KnownTypes{Dur: &durpb.Duration{Seconds: 1, Nanos: 0}}, false}, + {&pb.KnownTypes{Dur: &durpb.Duration{Seconds: -1, Nanos: 0}}, false}, + {&pb.KnownTypes{Dur: &durpb.Duration{Seconds: 1, Nanos: -1}}, true}, + {&pb.KnownTypes{Dur: &durpb.Duration{Seconds: -1, Nanos: 1}}, true}, + {&pb.KnownTypes{Dur: &durpb.Duration{Seconds: 1, Nanos: 1000000000}}, true}, + {&pb.KnownTypes{Dur: &durpb.Duration{Seconds: -1, Nanos: -1000000000}}, true}, + {&pb.KnownTypes{Ts: &tspb.Timestamp{Seconds: 1, Nanos: 1}}, false}, + {&pb.KnownTypes{Ts: &tspb.Timestamp{Seconds: 1, Nanos: -1}}, true}, + {&pb.KnownTypes{Ts: &tspb.Timestamp{Seconds: 1, Nanos: 1000000000}}, true}, } for _, tt := range tests { _, err := marshaler.MarshalToString(tt.pb) - if (err == nil) != (tt.errWant == nil) { - t.Errorf("marshaler.MarshalToString(%v) = _, %v; want _, %v", tt.pb, err, tt.errWant) - continue + if err == nil && tt.fail { + t.Errorf("marshaler.MarshalToString(%v) = _, ; want _, ", tt.pb) } - if err != nil && !strings.Contains(err.Error(), tt.errWant.Error()) { - t.Errorf("marshaler.MarshalToString(%v) = _, %v; want _, %v", tt.pb, err, tt.errWant) + if err != nil && !tt.fail { + t.Errorf("marshaler.MarshalToString(%v) = _, %v; want _, ", tt.pb, err) } } }