Publishing nanopb-0.2.1

If the null terminator is not present, string will be limited to the
data size of the field.

If you are still using the pb_enc_string (deprecated since 0.1.3) from
callbacks, now would be an excellent time to stop. The pb_field_t for
the callback will not contain proper data_size. Use pb_encode_string()
instead.

Update issue 68
Status: FixedInGit

CHANGELOG

@@ -1,3 +1,20 @@
+nanopb-0.2.1
+ NOTE: The default callback function signature has changed.
+       If you don't want to update your code, define PB_OLD_CALLBACK_STYLE.
+ 
+ Change the callback function to use void** (issue 69)
+ Add support for defining the nanopb options in a separate file (issue 12)
+ Add support for packed structs in IAR and MSVC (in addition to GCC) (issue 66)
+ Implement error message support for the encoder side (issue 7)
+ Handle unterminated strings when encoding (issue 68)
+ Fix bug with empty strings in repeated string callbacks (issue 73)
+ Fix regression in 0.2.0 with optional callback fields (issue 70)
+ Fix bugs with empty message types (issues 64, 65)
+ Fix some compiler warnings on clang (issue 67)
+ Some portability improvements (issues 60, 62)
+ Various new generator options
+ Improved tests
+
 nanopb-0.2.0
  NOTE: This release requires you to regenerate all .pb.c
        files. Files generated by older versions will not

generator/nanopb_generator.py

@@ -1,5 +1,5 @@
 '''Generate header file for nanopb from a ProtoBuf FileDescriptorSet.'''
-nanopb_version = "nanopb-0.2.1-dev"
+nanopb_version = "nanopb-0.2.1"
 
 try:
     import google.protobuf.descriptor_pb2 as descriptor

pb.h

@@ -6,7 +6,7 @@
  * see pb_encode.h or pb_decode.h
  */
 
-#define NANOPB_VERSION nanopb-0.2.1-dev
+#define NANOPB_VERSION nanopb-0.2.1
 
 #ifdef PB_SYSTEM_HEADER
 #include PB_SYSTEM_HEADER
@@ -30,7 +30,7 @@
 #   define PB_PACKED_STRUCT_START _Pragma("pack(push, 1)")
 #   define PB_PACKED_STRUCT_END _Pragma("pack(pop)")
 #   define pb_packed
-#elif defined(_MSC_VER)
+#elif defined(_MSC_VER) && (_MSC_VER >= 1500)
     /* For Microsoft Visual C++ */
 #   define PB_PACKED_STRUCT_START __pragma(pack(push, 1))
 #   define PB_PACKED_STRUCT_END __pragma(pack(pop))

pb_decode.c

@@ -331,6 +331,19 @@ static bool pb_field_next(pb_field_iterator_t *iter)
     return notwrapped;
 }
 
+static bool checkreturn pb_field_find(pb_field_iterator_t *iter, uint32_t tag)
+{
+    unsigned start = iter->field_index;
+    
+    do {
+        if (iter->pos->tag == tag)
+            return true;
+        pb_field_next(iter);
+    } while (iter->field_index != start);
+    
+    return false;
+}
+
 /*************************
  * Decode a single field *
  *************************/
@@ -417,11 +430,11 @@ static bool checkreturn decode_callback_field(pb_istream_t *stream, pb_wire_type
         if (!pb_make_string_substream(stream, &substream))
             return false;
         
-        while (substream.bytes_left)
+        do
         {
             if (!pCallback->funcs.decode(&substream, iter->pos, arg))
                 PB_RETURN_ERROR(stream, "callback failed");
-        }
+        } while (substream.bytes_left);
         
         pb_close_string_substream(stream, &substream);
         return true;
@@ -459,105 +472,71 @@ static bool checkreturn decode_field(pb_istream_t *stream, pb_wire_type_t wire_t
     }
 }
 
-/* Set field count to zero (or clear has_ field). */
-static void pb_clear_field_count(const pb_field_iterator_t *iter)
+/* Initialize message fields to default values, recursively */
+static void pb_message_set_to_defaults(const pb_field_t fields[], void *dest_struct)
 {
-    pb_type_t type;
-    type = iter->pos->type;
+    pb_field_iterator_t iter;
+    pb_field_init(&iter, fields, dest_struct);
     
-    if (iter->pos->tag == 0)
-        return; /* Empty message type */
-
-    if (PB_ATYPE(type) == PB_ATYPE_STATIC)
+    /* Initialize size/has fields and apply default values */
+    do
     {
-        if (PB_HTYPE(type) == PB_HTYPE_OPTIONAL)
+        pb_type_t type;
+        type = iter.pos->type;
+    
+        if (iter.pos->tag == 0)
+            continue;
+        
+        if (PB_ATYPE(type) == PB_ATYPE_STATIC)
         {
-            *(bool*)iter->pSize = false;
+            /* Initialize the size field for optional/repeated fields to 0. */
+            if (PB_HTYPE(type) == PB_HTYPE_OPTIONAL)
+            {
+                *(bool*)iter.pSize = false;
+            }
+            else if (PB_HTYPE(type) == PB_HTYPE_REPEATED)
+            {
+                *(size_t*)iter.pSize = 0;
+                continue; /* Array is empty, no need to initialize contents */
+            }
+            
+            /* Initialize field contents to default value */
+            if (PB_LTYPE(iter.pos->type) == PB_LTYPE_SUBMESSAGE)
+            {
+                pb_message_set_to_defaults((const pb_field_t *) iter.pos->ptr, iter.pData);
+            }
+            else if (iter.pos->ptr != NULL)
+            {
+                memcpy(iter.pData, iter.pos->ptr, iter.pos->data_size);
+            }
+            else
+            {
+                memset(iter.pData, 0, iter.pos->data_size);
+            }
         }
-        else if (PB_HTYPE(type) == PB_HTYPE_REPEATED)
+        else if (PB_ATYPE(type) == PB_ATYPE_CALLBACK)
         {
-            *(size_t*)iter->pSize = 0;
+            continue; /* Don't overwrite callback */
         }
-    }
-}
-
-/* Initialize message field to default value. Recurses on submessages. */
-static void pb_set_field_to_default(const pb_field_iterator_t *iter)
-{
-    pb_type_t type;
-    type = iter->pos->type;
-
-    if (iter->pos->tag == 0)
-        return; /* Empty message type */
-
-    /* We only need to initialize static fields.
-     * Furthermore, arrays do not need to be initialized as their length
-     * will be zero by default.
-     */
-    if (PB_ATYPE(type) == PB_ATYPE_STATIC &&
-        PB_HTYPE(type) != PB_HTYPE_REPEATED)
-    {
-        if (PB_LTYPE(iter->pos->type) == PB_LTYPE_SUBMESSAGE)
-        {
-            /* Submessage: initialize the fields recursively */
-            pb_field_iterator_t subiter;
-            pb_field_init(&subiter, (const pb_field_t *)iter->pos->ptr, iter->pData);
-            do {
-                pb_clear_field_count(&subiter);
-                pb_set_field_to_default(&subiter);
-            } while (pb_field_next(&subiter));
-        }
-        else if (iter->pos->ptr != NULL)
-        {
-            /* Normal field: copy the default value */
-            memcpy(iter->pData, iter->pos->ptr, iter->pos->data_size);
-        }
-        else
-        {
-            /* Normal field without default value: initialize to zero */
-            memset(iter->pData, 0, iter->pos->data_size);
-        }
-    }
+    } while (pb_field_next(&iter));
 }
 
 /*********************
  * Decode all fields *
  *********************/
 
-/* Helper function to initialize fields while advancing iterator */
-static void advance_iterator(pb_field_iterator_t *iter, bool *initialize, bool *current_seen)
-{
-    /* Initialize the fields we didn't decode. */
-    if (*initialize && !*current_seen)
-        pb_set_field_to_default(iter);
-    
-    /* Stop initializing after the first pass through the array */
-    if (!pb_field_next(iter))
-        *initialize = false;
-    
-    /* Clear the field count before decoding */
-    if (*initialize)
-        pb_clear_field_count(iter);
-    
-    /* Reset the flag to indicate that the new field has not been written to yet. */
-    *current_seen = false;
-}
-
-static bool checkreturn pb_decode_inner(pb_istream_t *stream, const pb_field_t fields[], void *dest_struct, bool initialize)
+bool checkreturn pb_decode_noinit(pb_istream_t *stream, const pb_field_t fields[], void *dest_struct)
 {
     uint8_t fields_seen[(PB_MAX_REQUIRED_FIELDS + 7) / 8] = {0}; /* Used to check for required fields */
-    bool current_seen = false;
     pb_field_iterator_t iter;
+    
     pb_field_init(&iter, fields, dest_struct);
-    pb_clear_field_count(&iter);
     
     while (stream->bytes_left)
     {
         uint32_t tag;
         pb_wire_type_t wire_type;
         bool eof;
-        unsigned start;
-        bool skip = false;
         
         if (!pb_decode_tag(stream, &wire_type, &tag, &eof))
         {
@@ -567,45 +546,20 @@ static bool checkreturn pb_decode_inner(pb_istream_t *stream, const pb_field_t f
                 return false;
         }
         
-        /* Go through the fields until we either find a match or
-         * wrap around to start. On the first pass, also initialize
-         * any missing fields.
-         * 
-         * The logic here is to avoid unnecessary initialization
-         * in the common case, where all fields occur in the proper
-         * order.
-         */
-        start = iter.field_index;
-        while (iter.pos->tag != tag)
-        {
-            advance_iterator(&iter, &initialize, &current_seen);
-            
-            if (iter.field_index == start)
-            {
-                skip = true;
-                break;
-            }
-        }
-        
-        /* Skip data if field was not found */
-        if (skip)
+        if (!pb_field_find(&iter, tag))
         {
+            /* No match found, skip data */
             if (!pb_skip_field(stream, wire_type))
                 return false;
-
             continue;
         }
         
-        current_seen = true;
-        
-        /* Keep track if all required fields are present */
         if (PB_HTYPE(iter.pos->type) == PB_HTYPE_REQUIRED
             && iter.required_field_index < PB_MAX_REQUIRED_FIELDS)
         {
             fields_seen[iter.required_field_index >> 3] |= (uint8_t)(1 << (iter.required_field_index & 7));
         }
             
-        /* Finally, decode the field data */
         if (!decode_field(stream, wire_type, &iter))
             return false;
     }
@@ -615,9 +569,6 @@ static bool checkreturn pb_decode_inner(pb_istream_t *stream, const pb_field_t f
         /* First figure out the number of required fields by
          * seeking to the end of the field array. Usually we
          * are already close to end after decoding.
-         *
-         * Note: this simultaneously initializes any fields
-         * that haven't been already initialized.
          */
         unsigned req_field_count;
         pb_type_t last_type;
@@ -625,8 +576,7 @@ static bool checkreturn pb_decode_inner(pb_istream_t *stream, const pb_field_t f
         do {
             req_field_count = iter.required_field_index;
             last_type = iter.pos->type;
-            advance_iterator(&iter, &initialize, &current_seen);
-        } while (iter.field_index != 0);
+        } while (pb_field_next(&iter));
         
         /* Fixup if last field was also required. */
         if (PB_HTYPE(last_type) == PB_HTYPE_REQUIRED && iter.pos->tag)
@@ -647,14 +597,10 @@ static bool checkreturn pb_decode_inner(pb_istream_t *stream, const pb_field_t f
     return true;
 }
 
-bool checkreturn pb_decode_noinit(pb_istream_t *stream, const pb_field_t fields[], void *dest_struct)
-{
-    return pb_decode_inner(stream, fields, dest_struct, false);
-}
-
 bool checkreturn pb_decode(pb_istream_t *stream, const pb_field_t fields[], void *dest_struct)
 {
-    return pb_decode_inner(stream, fields, dest_struct, true);
+    pb_message_set_to_defaults(fields, dest_struct);
+    return pb_decode_noinit(stream, fields, dest_struct);
 }
 
 /* Field decoders */
@@ -718,7 +664,8 @@ bool pb_decode_fixed64(pb_istream_t *stream, void *dest)
 bool checkreturn pb_dec_varint(pb_istream_t *stream, const pb_field_t *field, void *dest)
 {
     uint64_t value;
-    bool status = pb_decode_varint(stream, &value);
+    if (!pb_decode_varint(stream, &value))
+        return false;
     
     switch (field->data_size)
     {
@@ -729,13 +676,14 @@ bool checkreturn pb_dec_varint(pb_istream_t *stream, const pb_field_t *field, vo
         default: PB_RETURN_ERROR(stream, "invalid data_size");
     }
     
-    return status;
+    return true;
 }
 
 bool checkreturn pb_dec_svarint(pb_istream_t *stream, const pb_field_t *field, void *dest)
 {
     int64_t value;
-    bool status = pb_decode_svarint(stream, &value);
+    if (!pb_decode_svarint(stream, &value))
+        return false;
     
     switch (field->data_size)
     {
@@ -744,7 +692,7 @@ bool checkreturn pb_dec_svarint(pb_istream_t *stream, const pb_field_t *field, v
         default: PB_RETURN_ERROR(stream, "invalid data_size");
     }
     
-    return status;
+    return true;
 }
 
 bool checkreturn pb_dec_fixed32(pb_istream_t *stream, const pb_field_t *field, void *dest)
@@ -803,7 +751,12 @@ bool checkreturn pb_dec_submessage(pb_istream_t *stream, const pb_field_t *field
     if (field->ptr == NULL)
         PB_RETURN_ERROR(stream, "invalid field descriptor");
     
-    status = pb_decode(&substream, submsg_fields, dest);
+    /* New array entries need to be initialized, while required and optional
+     * submessages have already been initialized in the top-level pb_decode. */
+    if (PB_HTYPE(field->type) == PB_HTYPE_REPEATED)
+        status = pb_decode(&substream, submsg_fields, dest);
+    else
+        status = pb_decode_noinit(&substream, submsg_fields, dest);
     
     pb_close_string_substream(stream, &substream);
     return status;

pb_decode.h

@@ -9,7 +9,6 @@
  * These are usually generated from .proto-files with a script.
  */
 
-#include <stdbool.h>
 #include "pb.h"
 
 #ifdef __cplusplus
@@ -62,9 +61,6 @@ bool pb_decode(pb_istream_t *stream, const pb_field_t fields[], void *dest_struc
 /* Same as pb_decode, except does not initialize the destination structure
  * to default values. This is slightly faster if you need no default values
  * and just do memset(struct, 0, sizeof(struct)) yourself.
- *
- * It can also be used to merge fields from a new message into a previously
- * initialized structure.
  */
 bool pb_decode_noinit(pb_istream_t *stream, const pb_field_t fields[], void *dest_struct);
 

pb_encode.c

@@ -461,8 +461,16 @@ bool checkreturn pb_enc_bytes(pb_ostream_t *stream, const pb_field_t *field, con
 
 bool checkreturn pb_enc_string(pb_ostream_t *stream, const pb_field_t *field, const void *src)
 {
-    UNUSED(field);
-    return pb_encode_string(stream, (const uint8_t*)src, strlen((const char*)src));
+    /* strnlen() is not always available, so just use a for-loop */
+    size_t size = 0;
+    const char *p = (const char*)src;
+    while (size < field->data_size && *p != '\0')
+    {
+        size++;
+        p++;
+    }
+
+    return pb_encode_string(stream, (const uint8_t*)src, size);
 }
 
 bool checkreturn pb_enc_submessage(pb_ostream_t *stream, const pb_field_t *field, const void *src)

pb_encode.h

@@ -6,7 +6,6 @@
  * and their field descriptions (just like with pb_decode).
  */
 
-#include <stdbool.h>
 #include "pb.h"
 
 #ifdef __cplusplus

tests/callbacks.proto

@@ -11,5 +11,6 @@ message TestMessage {
     repeated fixed32 fixed32value = 3;
     repeated fixed64 fixed64value = 4;
     optional SubMessage submsg = 5;
+    repeated string repeatedstring = 6;
 }
 

tests/encode_unittests.c

@@ -180,12 +180,14 @@ int main()
     {
         uint8_t buffer[30];
         pb_ostream_t s;
-        char value[] = "xyzzy";
+        char value[30] = "xyzzy";
         
         COMMENT("Test pb_enc_string")
-        TEST(WRITES(pb_enc_string(&s, NULL, &value), "\x05xyzzy"))
+        TEST(WRITES(pb_enc_string(&s, &StringMessage_fields[0], &value), "\x05xyzzy"))
         value[0] = '\0';
-        TEST(WRITES(pb_enc_string(&s, NULL, &value), "\x00"))
+        TEST(WRITES(pb_enc_string(&s, &StringMessage_fields[0], &value), "\x00"))
+        memset(value, 'x', 30);
+        TEST(WRITES(pb_enc_string(&s, &StringMessage_fields[0], &value), "\x0Axxxxxxxxxx"))
     }
     
     {

tests/test_decode_callbacks.c

@@ -83,6 +83,8 @@ int main()
     testmessage.fixed32value.arg = "fixed32value: %ld\n";
     testmessage.fixed64value.funcs.decode = &print_fixed64;
     testmessage.fixed64value.arg = "fixed64value: %lld\n";
+    testmessage.repeatedstring.funcs.decode = &print_string;
+    testmessage.repeatedstring.arg = "repeatedstring: \"%s\"\n";
     
     if (!pb_decode(&stream, TestMessage_fields, &testmessage))
         return 1;

tests/test_encode_callbacks.c

@@ -41,6 +41,22 @@ bool encode_fixed64(pb_ostream_t *stream, const pb_field_t *field, void * const
     return pb_encode_fixed64(stream, &value);
 }
 
+bool encode_repeatedstring(pb_ostream_t *stream, const pb_field_t *field, void * const *arg)
+{
+    char *str[4] = {"Hello world!", "", "Test", "Test2"};
+    int i;
+    
+    for (i = 0; i < 4; i++)
+    {
+        if (!pb_encode_tag_for_field(stream, field))
+            return false;
+        
+        if (!pb_encode_string(stream, (uint8_t*)str[i], strlen(str[i])))
+            return false;
+    }
+    return true;
+}
+
 int main()
 {
     uint8_t buffer[1024];
@@ -57,6 +73,8 @@ int main()
     testmessage.submsg.int32value.funcs.encode = &encode_int32;
     testmessage.submsg.fixed32value.funcs.encode = &encode_fixed32;
     testmessage.submsg.fixed64value.funcs.encode = &encode_fixed64;
+
+    testmessage.repeatedstring.funcs.encode = &encode_repeatedstring;
     
     if (!pb_encode(&stream, TestMessage_fields, &testmessage))
         return 1;

tests/unittestproto.proto

@@ -8,6 +8,10 @@ message FloatArray {
     repeated float data = 1 [(nanopb).max_count = 10];
 }
 
+message StringMessage {
+    required string data = 1 [(nanopb).max_size = 10];
+}
+
 message CallbackArray {
     // We cheat a bit and use this message for testing other types, too.
     // Nanopb does not care about the actual defined data type for callback