To correctly handle this case we must add the serialized map entry to the unknown fields. Ideally we could merely preserve the map entry's serialized bytes from the input. However this is tricky to do if we are streaming and the previous buffer where the map entry began is no longer available.
This CL fixes this edge case by using the encoder to re-encode the map entry rather than using the input bytes directly.
While this fix is reasonably simple and reliable, it has two unfortunate properties. One is performance: we now must run the encoder to recreate bytes that we already saw in the input.
The other is dependencies: this fix has the unfortunate property of making the decoder depend on the encoder. In applications that only want the decoder but not the encoder, this will increase binary size. But the practical effects of this are probably minimal (the vast majority of applications that depend on the decoder will also use the encoder).
We can revisit this later and see if there is a better way of preserving the input bytes without re-encoding. For now this fix is simple and correct and fixes the fuzz bug.
PiperOrigin-RevId: 505381927
The initial motivation for this CL was to fix a bug found by fuzzing. But the fuzz bug pointed out a few edge cases that this CL corrects:
1. The core bug is that we were allowing a map entry sub-message to be linked to a group field. This is not allowed in protobuf schemas, but we did not check for this edge case in `upb_MiniTable_SetSubMessage()`, so we were de facto allowing it. This triggered some bad behavior in the parser whereby we pushed a limit without checking its validity first.
2. To defend against this, I added asserts in `upb_MiniTable_SetSubMessage()` to verify the type of the field we are linking, to ensure that a group field is not linked to a map entry sub-message. But this should probably be changed to return an error instead of relying on asserts for this.
3. I changed the fuzz util code that builds the MiniTable so that it will never violate this new invariant. The fuzz util code now can run into situations where a group field has no valid non-map-entry sub-message to select. In those cases it will simply not register any sub-message for that field.
4. Previously group did not support leaving sub-messages unregistered. Previously I added this feature for sub-messages but not for groups. There is no reason why dynamic tree shaking should not work for group fields, so I extended the feature to support groups also.
PiperOrigin-RevId: 504913630
According to https://en.cppreference.com/w/c/program/setjmp automatic variables
modified in a function calling setjmp can have indeterminate values. Instead,
refactor all functions calling setjmp so that the function calling setjmp
doesn’t have any local variables.
Part VI: Wire encoder/decoder.
PiperOrigin-RevId: 504810940
The overall motivation for this interface is to consolidate many places in upb that are parsing wire format data directly.
This interface is not yet complete, but this is a good start. We have enough to port the wire format parsing in accessors.c to this interface. We can follow up by porting more places that do wire format parsing.
PiperOrigin-RevId: 498109788
Moving the logic down to EpsCopyInputStream makes it easier to test and reuse this functionality.
We also implement aliasing for the final bytes of the patch buffer, which has never been supported before. We used to always force a copy for any data parsed out of the patch buffer at the end of the stream.
Much of this logic is ported directly from the C++ EpsCopyInputStream class.
PiperOrigin-RevId: 498091644
This mirrors the structure of C++ protobuf, which has an EpsCopyInputStream class.
This will lay the foundation for making EpsCopyInputStream capable of true streaming, by reading its input from a ZeroCopyInputStream. It also lets us test EpsCopyInputStream separately from the decoder: see the new unit test that fuzzes upb_EpsCopyInputStream.
After this CL is submitted, the two decoders (the normal decoder and the fast decoder) should no longer be accessing the members of upb_EpsCopyInputStream.
PiperOrigin-RevId: 494400285
This required some work to unify map entry messages with regular messages, with respect to presence. Before map entry fields could never have presence. Now they can have presence according to normal rules. Note that this only applies to times that the user constructs a map entry directly.
PiperOrigin-RevId: 490611656
Prior to this CL, there were several different code paths for reading/writing message data. Generated code, MiniTable accessors, and reflection all performed direct manipulation of the bits and bytes in a message, but they all had distinct implementations that did not share much of any code. This divergence meant that they could easily have different behavior, bugs could creep into one but not another, and we would need three different sets of tests to get full test coverage. This also made it very difficult to change the internal representation in any way, since it would require updating many places in the code.
With this CL, the three different APIs for accessing message data now all share a common set of functions. The common functions all take a `upb_MiniTableField` as the canonical description of a field's type and layout. The lowest-level functions are very branchy, as they must test for every possible variation in the field type (field vs oneof, hasbit vs no-hasbit, different field sizes, whether a nonzero default value exists, extension vs. regular field), however these functions are declared inline and designed to be very optimizable when values are known at compile time.
In generated accessors, for example, we can declare constant `upb_MiniTableField` instances so that all values can constant-propagate, and we can get fully specialized code even though we are calling a generic function. On the other hand, when we use the generic functions from reflection, we get runtime branches since values are not known at compile time. But even the function is written to still be as efficient as possible even when used from reflection. For example, we use memcpy() calls with constant length so that the compiler can optimize these into inline loads/stores without having to make an out-of-line call to memcpy().
In this way, this CL should be a benefit to both correctness and performance. It will also make it easier to change the message representation, for example to optimize the encoder by giving hasbits to all fields.
Note that we have not completely consolidated all access in this CL:
1. Some functions outside of get/set such as clear and hazzers are not yet unified.
2. The encoder and decoder still touch the message without going through the common functions. The encoder and decoder require a bit more specialized code to get good performance when reading/writing fields en masse.
PiperOrigin-RevId: 490016095
Remove circular dependencies that were bouncing back and forth between
msg_internal.h and mini_table/, including:
- splitting out each mini table subtype into its own header
- moving the non-reflection message code into message/
- moving the accessors from mini_table/ to message/
PiperOrigin-RevId: 489121042
- replace all instances of the deprecated iterator with the much nicer new one
- fix a bug which caused the new iterator to skip all values in the hash array
- fix a bug which caused the new iterator to skip the first value in the hash table
- delete the old iterator code
- also replace most uses of the deprecated string hash table iterator
PiperOrigin-RevId: 489093240
There are several other functions which might eventually end up here and ideally become unified across json/ and text/ and io/ so this is just a first step to create the new subdir and get rid of upb/internal/
PiperOrigin-RevId: 488954926
Move the map-related functions from msg_internal.h that are only used in generated code into map_gencode_util.h. Then move the rest of the map-related functions from msg_internal.h into map_internal.h.
PiperOrigin-RevId: 486299140
Joshua Haberman
Protobuf Team Bot
Eric Salo
Mike Kruskal