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.

diff --git a/docs/concepts.rst b/docs/concepts.rst
index f5e32ad..1d0fe08 100644 (file)
--- a/docs/concepts.rst
+++ b/docs/concepts.rst
@@ -158,7 +158,10 @@ required bytes data = 1 [(nanopb).max_size = 40];
                                                                                 | 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
 ===============

diff --git a/generator/nanopb_generator.py b/generator/nanopb_generator.py
index e1d99c9..2ceafc7 100644 (file)
--- a/generator/nanopb_generator.py
+++ b/generator/nanopb_generator.py
@@ -219,9 +219,6 @@ class Field:
             result += '\n    pb_membersize(%s, %s[0]),' % (self.struct_name, self.name)
             result += ('\n    pb_membersize(%s, %s) / pb_membersize(%s, %s[0]),'
                        % (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:
             result += '\n    pb_membersize(%s, %s),' % (self.struct_name, self.name)
             result += ' 0,'

diff --git a/pb_decode.c b/pb_decode.c
index 3992ab8..1e2fea0 100644 (file)
--- a/pb_decode.c
+++ b/pb_decode.c
@@ -509,7 +509,8 @@ bool checkreturn pb_dec_bytes(pb_istream_t *stream, const pb_field_t *field, voi
         return false;
     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 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))
         return false;
     
+    /* Check length, noting the null terminator */
     if (size > field->data_size - 1)
         return false;
     

diff --git a/tests/decode_unittests.c b/tests/decode_unittests.c
index ab12ac3..6ba6d4f 100644 (file)
--- a/tests/decode_unittests.c
+++ b/tests/decode_unittests.c
@@ -167,14 +167,22 @@ int main()
     {
         pb_istream_t s;
         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")
         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("\x06xxxxxx"), !pb_dec_bytes(&s, &f, &d)))
         TEST((s = S("\x05xxxxx"), pb_dec_bytes(&s, &f, &d) && d.size == 5))
         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)))
     }
     
     {