Unmarshaling errors for packed fields

[awalterschulze]

protoc --decode-raw does not return any error, but golang/protobuf does.

Given the following message

message NinRepPackedNative {
  repeated double Field1 = 1 [packed = true];
  repeated float Field2 = 2 [packed = true];
  repeated int32 Field3 = 3 [packed = true];
  repeated int64 Field4 = 4 [packed = true];
  repeated uint32 Field5 = 5 [packed = true];
  repeated uint64 Field6 = 6 [packed = true];
  repeated sint32 Field7 = 7 [packed = true];
  repeated sint64 Field8 = 8 [packed = true];
  repeated fixed32 Field9 = 9 [packed = true];
  repeated sfixed32 Field10 = 10 [packed = true];
  repeated fixed64 Field11 = 11 [packed = true];
  repeated sfixed64 Field12 = 12 [packed = true];
  repeated bool Field13 = 13 [packed = true];
}

Given the following inputs

[]byte{0x6a, 0x16, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0xb9, 0x30}

returns bad wiretype for Field4

[]byte{0x6a, 0x16, 0x30, 0xfc, 0x30, 0x30, 0x30, 0x30, 0xf6, 0xfa, 0x30, 0x30, 0x30, 0xc1, 0xaf, 0xf5, 0x30, 0x30, 0x30, 0xcf, 0x30, 0x30, 0xb9, 0x30, 0x7a, 0xd, 0x30, 0x85, 0x30, 0xd3, 0x30, 0x30, 0x30, 0xa8, 0x30, 0xa7, 0x30, 0x30, 0x30}

returns unexpected eof

[]byte{0x6a, 0x16, 0x30, 0xfc, 0x30, 0x30, 0x30, 0x30, 0xf6, 0xfa, 0x30, 0x30, 0x30, 0xc1, 0xaf, 0xf5, 0x30, 0x30, 0x30, 0xcf, 0x30, 0x30, 0xb9, 0x30, 0x7a, 0xd, 0x30, 0x85, 0x30, 0xd3, 0x30, 0x30, 0x30, 0x30, 0x27, 0x30, 0x30, 0x30, 0x30}

returns cant skip wiretype

These were found using go-fuzz @dvyukov
Using this fuzz test
https://github.com/gogo/fuzztests/tree/e62df375312286dd3e1e65abffef3a3407dcd6b0

[dsymonds]

Odd input 1. Field4 has tag 4, wire 2, which means its header should be 0x24. I don't see that byte anywhere. The first byte, 0x6a, is 13<<3|2, which is for Field13, followed by 0x16 (len=22), followed by 22 bytes. That should code into Field13, and nothing else. Are you sure proto.Unmarshal is saying bad wiretype for Field4?

[awalterschulze]

Yes my generated code and protoc decode-raw decodes those bytes into

&fuzztests.NinRepPackedNative{Field13: []bool{true, true, true, true, true, true, true, true, true, true, true, true, true, true},
        XXX_unrecognized:[]byte{0x7a, 0xd, 0x30, 0x85, 0x30, 0xd3, 0x30, 0x30, 0x30, 0x30, 0x27, 0x30, 0x30, 0x30, 0x30},
        }

, but the reflected/unsafe code returns this error

proto: bad wiretype for field fuzztests.NinRepPackedNative.Field4: got wiretype 7, want 0

[awalterschulze]

I have those tests here and ready to go
https://github.com/gogo/protobuf/tree/master/test/fuzztests
Simply change the Makefile to go_out instead of gogofast_out and then enable the disabled tests

• BadWireType
• UnexpectedEOF
• CantSkipWireType
  They are written to expected an error, which is a bit weird, but I initially thought there was something wrong with my generated code.

[dsymonds]

The first input,

[]byte{0x6a, 0x16, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0xb9, 0x30}

is valid. The Go code is incorrectly returning unexpected EOF. 0x6a is 13<<3|2 (field 13, with length-delimited data), followed by 0x16 (22 bytes), but only 21 values (the last is a two byte varint bool; stupid, but valid). dec_slice_packed_bool is at fault; it is counting the number of values it reads, not the number of bytes.

[dsymonds]

0c959e8 deals with dec_slice_packed_bool. I'll take a look at the next one now.

[dsymonds]

The second input ([]byte{0x6a, 0x16, 0x30, 0xfc, 0x30, 0x30, 0x30, 0x30, 0xf6, 0xfa, 0x30, 0x30, 0x30, 0xc1, 0xaf, 0xf5, 0x30, 0x30, 0x30, 0xcf, 0x30, 0x30, 0xb9, 0x30, 0x7a, 0xd, 0x30, 0x85, 0x30, 0xd3, 0x30, 0x30, 0x30, 0xa8, 0x30, 0xa7, 0x30, 0x30, 0x30}) now parses without error. Its prefix was tripping the same bug in dec_slice_packed_bool.

[dsymonds]

The third input ([]byte{0x6a, 0x16, 0x30, 0xfc, 0x30, 0x30, 0x30, 0x30, 0xf6, 0xfa, 0x30, 0x30, 0x30, 0xc1, 0xaf, 0xf5, 0x30, 0x30, 0x30, 0xcf, 0x30, 0x30, 0xb9, 0x30, 0x7a, 0xd, 0x30, 0x85, 0x30, 0xd3, 0x30, 0x30, 0x30, 0x30, 0x27, 0x30, 0x30, 0x30, 0x30}) now parses without error too, and I believe it was also tripping the dec_slice_packed_bool bug.

I think everything is fixed here. Thanks for the report!

[awalterschulze]

My pleasure :)
Thanks go-fuzz @dvyukov
I suspect there is another bug where golang/protobuf is throwing away unrecognized bytes.

You should be able to reproduce it using some quick editing of my fuzztests Fuzz function.
Basically I think what happens is there is some unrecognised bytes and those bytes contain some error that jump you across the total buffer length. All those bytes are thrown away.

Or you can wait a few months for when I get the time to do the inefficient varint detecting to properly catch those bugs.

[awalterschulze]

fuzz tests for packed fields are looking good 👍