Fixed issue 1 reported by Erik Rosen:

The size of non-callback bytes-fields was miscalculated, which
caused all following fields in a message to contain garbage.

Previous commit contains a testcase for this.

This fix changes the generated message description. If your protocol uses
bytes-fields, you should regenerate *.pb.c.
This commit is contained in:
Petteri Aimonen
2012-01-12 19:08:05 +02:00
parent 0f6b615ae3
commit 113bd7ee87
4 changed files with 17 additions and 7 deletions

View File

@@ -158,7 +158,10 @@ required bytes data = 1 [(nanopb).max_size = 40];
| Person_data_t data; | Person_data_t data;
=============================================================================== ======================= =============================================================================== =======================
The maximum lengths are checked in runtime. If string/bytes/array exceeds the allocated length, *pb_decode* will return false. The maximum lengths are checked in runtime. If string/bytes/array exceeds the allocated length, *pb_decode* will return false.
Note: for the *bytes* datatype, the field length checking may not be exact.
The compiler may add some padding to the *pb_bytes_t* structure, and the nanopb runtime doesn't know how much of the structure size is padding. Therefore it uses the whole length of the structure for storing data, which is not very smart but shouldn't cause problems. In practise, this means that if you specify *(nanopb).max_size=5* on a *bytes* field, you may be able to store 6 bytes there. For the *string* field type, the length limit is exact.
Field callbacks Field callbacks
=============== ===============

View File

@@ -219,9 +219,6 @@ class Field:
result += '\n pb_membersize(%s, %s[0]),' % (self.struct_name, self.name) result += '\n pb_membersize(%s, %s[0]),' % (self.struct_name, self.name)
result += ('\n pb_membersize(%s, %s) / pb_membersize(%s, %s[0]),' result += ('\n pb_membersize(%s, %s) / pb_membersize(%s, %s[0]),'
% (self.struct_name, self.name, self.struct_name, self.name)) % (self.struct_name, self.name, self.struct_name, self.name))
elif self.htype != 'PB_HTYPE_CALLBACK' and self.ltype == 'PB_LTYPE_BYTES':
result += '\n pb_membersize(%s, bytes),' % self.ctype
result += ' 0,'
else: else:
result += '\n pb_membersize(%s, %s),' % (self.struct_name, self.name) result += '\n pb_membersize(%s, %s),' % (self.struct_name, self.name)
result += ' 0,' result += ' 0,'

View File

@@ -509,7 +509,8 @@ bool checkreturn pb_dec_bytes(pb_istream_t *stream, const pb_field_t *field, voi
return false; return false;
x->size = temp; x->size = temp;
if (x->size > field->data_size) /* Check length, noting the space taken by the size_t header. */
if (x->size > field->data_size - offsetof(pb_bytes_array_t, bytes))
return false; return false;
return pb_read(stream, x->bytes, x->size); return pb_read(stream, x->bytes, x->size);
@@ -522,6 +523,7 @@ bool checkreturn pb_dec_string(pb_istream_t *stream, const pb_field_t *field, vo
if (!pb_decode_varint32(stream, &size)) if (!pb_decode_varint32(stream, &size))
return false; return false;
/* Check length, noting the null terminator */
if (size > field->data_size - 1) if (size > field->data_size - 1)
return false; return false;

View File

@@ -167,14 +167,22 @@ int main()
{ {
pb_istream_t s; pb_istream_t s;
struct { size_t size; uint8_t bytes[5]; } d; struct { size_t size; uint8_t bytes[5]; } d;
pb_field_t f = {1, PB_LTYPE_BYTES, 0, 0, 5, 0, 0}; pb_field_t f = {1, PB_LTYPE_BYTES, 0, 0, sizeof(d), 0, 0};
COMMENT("Test pb_dec_bytes") COMMENT("Test pb_dec_bytes")
TEST((s = S("\x00"), pb_dec_bytes(&s, &f, &d) && d.size == 0)) TEST((s = S("\x00"), pb_dec_bytes(&s, &f, &d) && d.size == 0))
TEST((s = S("\x01\xFF"), pb_dec_bytes(&s, &f, &d) && d.size == 1 && d.bytes[0] == 0xFF)) TEST((s = S("\x01\xFF"), pb_dec_bytes(&s, &f, &d) && d.size == 1 && d.bytes[0] == 0xFF))
TEST((s = S("\x06xxxxxx"), !pb_dec_bytes(&s, &f, &d)))
TEST((s = S("\x05xxxxx"), pb_dec_bytes(&s, &f, &d) && d.size == 5)) TEST((s = S("\x05xxxxx"), pb_dec_bytes(&s, &f, &d) && d.size == 5))
TEST((s = S("\x05xxxx"), !pb_dec_bytes(&s, &f, &d))) TEST((s = S("\x05xxxx"), !pb_dec_bytes(&s, &f, &d)))
/* Note: the size limit on bytes-fields is not strictly obeyed, as
* the compiler may add some padding to the struct. Using this padding
* is not a very good thing to do, but it is difficult to avoid when
* we use only a single uint8_t to store the size of the field.
* Therefore this tests against a 10-byte string, while otherwise even
* 6 bytes should error out.
*/
TEST((s = S("\x10xxxxxxxxxx"), !pb_dec_bytes(&s, &f, &d)))
} }
{ {