facebook / fbthrift

Facebook's branch of Apache Thrift, including a new C++ server.
Apache License 2.0
2.57k stars 608 forks source link

Compact protocol specification off-by-one error #531

Closed terrelln closed 1 year ago

terrelln commented 1 year ago

The compact protocol specification seems to have a slight error in the definition of list/set encoding:

https://github.com/facebook/fbthrift/blob/main/thrift/doc/spec/protocol/data.md#listset-1

Specifically this paragraph:

Lists and sets are serialized in exactly the same way. The first byte contains the length (if less than 16) and the type code of the inner elements. If the length is at least 16, then it is encoded as a variable length integer. The serialized elements come next. First, the type code of the inner elements is written, then the 32-bit length, then the elements.

It states that a variable length integer if the length is at least 16, but the variable length integer must also be present if the length is 15. It would just be zero.

vitaut commented 1 year ago

Good catch, thanks. I plan to change this part of the spec to the following:

Lists and sets are serialized as follows. If the length is less than 15, then the first byte contains the length and the element type code. If the length is 15 or more, then the first byte contain 0xF and the element type code followed by the length serialized as a varint-encoded 32-bit integer. Serialized elements come next.

Note that we write the length as varint, not 0, in case it is equal to 15.

terrelln commented 1 year ago

Awesome, thanks for the fix!