Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

jsonpb: fix handling of illegal and negative nanoseconds #492

Merged
merged 4 commits into from
Jan 23, 2018
Merged

jsonpb: fix handling of illegal and negative nanoseconds #492

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0a2722a
fix handling of illegal and negative durations
dfawley Jan 22, 2018
e35b0e1
fix existing test Errorf problem
dfawley Jan 22, 2018
7437c35
add validation for ns range
dfawley Jan 22, 2018
96b08e6
remove error string testing
dfawley Jan 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions jsonpb/jsonpb.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -201,8 +203,16 @@ 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 ns <= -secondInNanos || ns >= secondInNanos {
return fmt.Errorf("ns out of range (%v, %v)", -secondInNanos, secondInNanos)
}
if (s > 0 && ns < 0) || (s < 0 && ns > 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check seems wrong. The documentation for Timestamp.nanos says:

Non-negative fractions of a second at nanosecond resolution. Negative second values with fractions must still have non-negative nanos values that count forward in time. Must be from 0 to 999,999,999 inclusive.

Which seems to indicate a negative value for ns is invalid regardless of s value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sigh. The spec here says the opposite:

https://developers.google.com/protocol-buffers/docs/reference/google.protobuf#google.protobuf.Duration

“For durations of one second or more, a non-zero value for the nanos field must be of the same sign as the seconds field. Must be from -999,999,999 to +999,999,999 inclusive.”

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wait. This is Duration, not Timestamp....

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, my mistake. I'm looking at Timestamp.

Also, to what degree should we validate durations?

  • Should we also verify that they are within -999999999 to +999999999 inclusive? Note that the logic below breaks if the value exceeds those bounds.
  • If we're going to invalidate bad Durations here, should we be consistent and apply similar checks to Timestamps down below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, to what degree should we validate durations?

I'll leave that up to you. If you want this CL to be consistent with the rest of the code, I can remove the validation and testing of illegal inputs here. If you want validation elsewhere, I would rather change the other places in a different commit, as this commit fixes a very real regression and it would be best to get a fix in ASAP.

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(`"`)
Expand All @@ -217,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")
Expand Down
32 changes: 31 additions & 1 deletion jsonpb/jsonpb_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ package jsonpb
import (
"bytes"
"encoding/json"
"errors"
"io"
"math"
"reflect"
Expand Down Expand Up @@ -407,6 +408,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"}},
Expand Down Expand Up @@ -473,6 +476,33 @@ func TestMarshalingNil(t *testing.T) {
}
}

func TestMarshalIllegalTime(t *testing.T) {
tests := []struct {
pb proto.Message
errWant error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we care that much about the exact error message. Let's just change this to a "fail bool" that checks whether it passes or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}{
{&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")},
}
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 && !strings.Contains(err.Error(), tt.errWant.Error()) {
t.Errorf("marshaler.MarshalToString(%v) = _, %v; want _, %v", tt.pb, err, tt.errWant)
}
}
}

func TestMarshalJSONPBMarshaler(t *testing.T) {
rawJson := `{ "foo": "bar", "baz": [0, 1, 2, 3] }`
msg := dynamicMessage{rawJson: rawJson}
Expand Down Expand Up @@ -920,7 +950,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)
}
}

Expand Down