From 48f5dd83530a46dd2423277f4488fd4ce9c93b72 Mon Sep 17 00:00:00 2001 From: Petteri Aimonen Date: Sat, 31 Dec 2016 10:33:48 +0200 Subject: [PATCH] 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. --- generator/nanopb_generator.py | 20 +++++++++++++++----- pb.h | 2 ++ pb_common.c | 6 +++--- 3 files changed, 20 insertions(+), 8 deletions(-) diff --git a/generator/nanopb_generator.py b/generator/nanopb_generator.py index a2ee22d..6e5ebaf 100755 --- 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 --- 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 --- 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) -- 2.16.6