Fix memory leak with oneofs and PB_ENABLE_MALLOC (#615)
Nanopb would leak memory when all of the following conditions were true:
- PB_ENABLE_MALLOC is defined at the compile time
- Message definitions contains an oneof field,
the oneof contains a static submessage, and
the static submessage contains a pointer field.
- Data being decoded contains two values for the submessage.
The logic in pb_release_union_field would detect that the same
submessage occurs twice, and wouldn't release it because keeping
the old values is necessary to match the C++ library behavior
regarding message merges.
But then decode_static_field() would go to memset() the whole
submessage to zero, because it unconditionally assumed it to
be uninitialized memory. This would normally happen when the
contents of the union field is switched to a different oneof
item, instead of merging with the same one.
This commit changes it so that the field is memset() only when
`which_field` contains a different tag. Also the setting of the
default values for the submessage was moved to decode_static_field()
so that it wouldn't overwrite the values that must be merged.
Test cases must still be extended to cover this problem.
diff --git a/pb_decode.c b/pb_decode.c
index 4f11c9d..28f6b57 100644
--- a/pb_decode.c
+++ b/pb_decode.c
@@ -31,6 +31,7 @@
static bool checkreturn decode_field(pb_istream_t *stream, pb_wire_type_t wire_type, pb_field_iter_t *field);
static bool checkreturn default_extension_decoder(pb_istream_t *stream, pb_extension_t *extension, uint32_t tag, pb_wire_type_t wire_type);
static bool checkreturn decode_extension(pb_istream_t *stream, uint32_t tag, pb_wire_type_t wire_type, pb_extension_t *extension);
+static bool pb_field_set_to_default(pb_field_iter_t *field);
static bool pb_message_set_to_defaults(pb_field_iter_t *iter);
static bool checkreturn pb_dec_bool(pb_istream_t *stream, const pb_field_iter_t *field);
static bool checkreturn pb_dec_varint(pb_istream_t *stream, const pb_field_iter_t *field);
@@ -516,8 +517,8 @@
}
case PB_HTYPE_ONEOF:
- *(pb_size_t*)field->pSize = field->tag;
- if (PB_LTYPE_IS_SUBMSG(field->type))
+ if (PB_LTYPE_IS_SUBMSG(field->type) &&
+ *(pb_size_t*)field->pSize != field->tag)
{
/* We memset to zero so that any callbacks are set to NULL.
* This is because the callbacks might otherwise have values
@@ -527,7 +528,12 @@
* that can set the fields before submessage is decoded.
* pb_dec_submessage() will set any default values. */
memset(field->pData, 0, (size_t)field->data_size);
+
+ /* Set default values for the submessage fields. */
+ if (!pb_field_set_to_default(field))
+ PB_RETURN_ERROR(stream, "failed to set defaults");
}
+ *(pb_size_t*)field->pSize = field->tag;
return decode_basic_field(stream, wire_type, field);
@@ -1580,8 +1586,7 @@
/* Static required/optional fields are already initialized by top-level
* pb_decode(), no need to initialize them again. */
if (PB_ATYPE(field->type) == PB_ATYPE_STATIC &&
- PB_HTYPE(field->type) != PB_HTYPE_REPEATED &&
- PB_HTYPE(field->type) != PB_HTYPE_ONEOF)
+ PB_HTYPE(field->type) != PB_HTYPE_REPEATED)
{
flags = PB_DECODE_NOINIT;
}
