Skip to content

Commit

Permalink
proto: do not allow unknown fields to satisfy required field bit (#565)
Browse files Browse the repository at this point in the history
A prior changed (see #511) allows Unmarshal to treat fields with
unknown wire types with known tags as unknown fields.
When doing so, we must be careful not to set the required field bit.

For example, suppose we have:
	message Foo { require fixed32 my_field = 1; }

Then parsing a message with a field of type varint and tag 1 should
not satisfy the required that Foo.my_field be set.
  • Loading branch information
dsnet authored Mar 28, 2018
1 parent 9c8fb7a commit 91cccdb
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 2 deletions.
19 changes: 19 additions & 0 deletions proto/all_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1860,6 +1860,25 @@ func TestRequiredNotSetError(t *testing.T) {
}
}

func TestRequiredNotSetErrorWithBadWireTypes(t *testing.T) {
// Required field expects a varint, and properly found a varint.
if err := Unmarshal([]byte{0x08, 0x00}, new(GoEnum)); err != nil {
t.Errorf("Unmarshal = %v, want nil", err)
}
// Required field expects a varint, but found a fixed32 instead.
if err := Unmarshal([]byte{0x0d, 0x00, 0x00, 0x00, 0x00}, new(GoEnum)); err == nil {
t.Errorf("Unmarshal = nil, want RequiredNotSetError")
}
// Required field expects a varint, and found both a varint and fixed32 (ignored).
m := new(GoEnum)
if err := Unmarshal([]byte{0x08, 0x00, 0x0d, 0x00, 0x00, 0x00, 0x00}, m); err != nil {
t.Errorf("Unmarshal = %v, want nil", err)
}
if !bytes.Equal(m.XXX_unrecognized, []byte{0x0d, 0x00, 0x00, 0x00, 0x00}) {
t.Errorf("expected fixed32 to appear as unknown bytes: %x", m.XXX_unrecognized)
}
}

func fuzzUnmarshal(t *testing.T, data []byte) {
defer func() {
if e := recover(); e != nil {
Expand Down
5 changes: 3 additions & 2 deletions proto/table_unmarshal.go
Original file line number Diff line number Diff line change
Expand Up @@ -166,20 +166,21 @@ func (u *unmarshalInfo) unmarshal(m pointer, b []byte) error {
} else {
f = u.sparse[tag]
}
reqMask |= f.reqMask
if fn := f.unmarshal; fn != nil {
var err error
b, err = fn(b, m.offset(f.field), wire)
if err == nil {
reqMask |= f.reqMask
continue
}
if r, ok := err.(*RequiredNotSetError); ok {
// Remember this error, but keep parsing. We need to produce
// a full parse even if a required field is missing.
rnse = r
reqMask |= f.reqMask
continue
}
if err == nil || err != errInternalBadWireType {
if err != errInternalBadWireType {
return err
}
// Fragments with bad wire type are treated as unknown fields.
Expand Down

0 comments on commit 91cccdb

Please # to comment.