Fix multiple oneofs in same message (issue #229)

Previously the field iterator logic didn't know whether two
oneof fields were part of the same union, or separate. This
caused wrong pointers to be calculated if multiple oneofs were
inside a single message.

This commit fixes this by using dataoffset of PB_SIZE_MAX to
indicate union fields after the first field.

Theoretically PB_SIZE_MAX is also a valid value for data offset,
which could cause errors. Adding a compile-time assert for this
is somewhat difficult. However I consider it extremely unlikely
that there is any platform that could trigger this situation, as
it would require 255 bytes of extra data/padding between two protobuf
oneof fields. On 64-bit architectures the worst case is 16 bytes,
and even esoteric platforms only align to 64 bytes or so. Manual
modification of the generated .pb.h file could trigger this, but
even then it would require pretty bad luck to happen.

diff --git a/generator/nanopb_generator.py b/generator/nanopb_generator.py
index a2ee22d..6e5ebaf 100755 (executable)
--- a/generator/nanopb_generator.py
+++ b/generator/nanopb_generator.py
@@ -509,9 +509,10 @@ class Field:
         identifier = '%s_%s_tag' % (self.struct_name, self.name)
         return '#define %-40s %d\n' % (identifier, self.tag)
 
-    def pb_field_t(self, prev_field_name):
+    def pb_field_t(self, prev_field_name, union_index = None):
         '''Return the pb_field_t initializer to use in the constant array.
-        prev_field_name is the name of the previous field or None.
+        prev_field_name is the name of the previous field or None. For OneOf
+        unions, union_index is the index of this field inside the OneOf.
         '''
 
         if self.rules == 'ONEOF':
@@ -526,7 +527,14 @@ class Field:
         result += '%-8s, ' % self.pbtype
         result += '%s, ' % self.rules
         result += '%-8s, ' % (self.allocation if not self.inline else "INLINE")
-        result += '%s, ' % ("FIRST" if not prev_field_name else "OTHER")
+        
+        if union_index is not None and union_index > 0:
+            result += 'UNION, '
+        elif prev_field_name is None:
+            result += 'FIRST, '
+        else:
+            result += 'OTHER, '
+        
         result += '%s, ' % self.struct_name
         result += '%s, ' % self.name
         result += '%s, ' % (prev_field_name or self.name)
@@ -767,8 +775,10 @@ class OneOf(Field):
         return ''.join([f.tags() for f in self.fields])
 
     def pb_field_t(self, prev_field_name):
-        result = ',\n'.join([f.pb_field_t(prev_field_name) for f in self.fields])
-        return result
+        parts = []
+        for union_index, field in enumerate(self.fields):
+            parts.append(field.pb_field_t(prev_field_name, union_index))
+        return ',\n'.join(parts)
 
     def get_last_field_name(self):
         if self.anonymous:

diff --git a/pb.h b/pb.h
index 2cf5c4d..f68d1d6 100644 (file)
--- a/pb.h
+++ b/pb.h
@@ -393,6 +393,8 @@ struct pb_extension_s {
 #define PB_DATAOFFSET_FIRST(st, m1, m2) (offsetof(st, m1))
 /* data_offset for subsequent fields */
 #define PB_DATAOFFSET_OTHER(st, m1, m2) (offsetof(st, m1) - offsetof(st, m2) - pb_membersize(st, m2))
+/* data offset for subsequent fields inside an union (oneof) */
+#define PB_DATAOFFSET_UNION(st, m1, m2) (PB_SIZE_MAX)
 /* Choose first/other based on m1 == m2 (deprecated, remains for backwards compatibility) */
 #define PB_DATAOFFSET_CHOOSE(st, m1, m2) (int)(offsetof(st, m1) == offsetof(st, m2) \
                                   ? PB_DATAOFFSET_FIRST(st, m1, m2) \

diff --git a/pb_common.c b/pb_common.c
index 385c019..4fb7186 100644 (file)
--- a/pb_common.c
+++ b/pb_common.c
@@ -42,11 +42,11 @@ bool pb_field_iter_next(pb_field_iter_t *iter)
         size_t prev_size = prev_field->data_size;
     
         if (PB_HTYPE(prev_field->type) == PB_HTYPE_ONEOF &&
-            PB_HTYPE(iter->pos->type) == PB_HTYPE_ONEOF)
+            PB_HTYPE(iter->pos->type) == PB_HTYPE_ONEOF &&
+            iter->pos->data_offset == PB_SIZE_MAX)
         {
             /* Don't advance pointers inside unions */
-            prev_size = 0;
-            iter->pData = (char*)iter->pData - prev_field->data_offset;
+            return true;
         }
         else if (PB_ATYPE(prev_field->type) == PB_ATYPE_STATIC &&
                  PB_HTYPE(prev_field->type) == PB_HTYPE_REPEATED)