Fixed SEGV in sub-message getters for well-known types when message is unset. #8670
+50
−28
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The well-known types generate C code into wkt.inc, and this C code was not testing
isset($msg->submsg_field)
like the generated code does:This led to an error in well-known type getters like
getListValue()
above where we would try to get a sub-message field from upb whenupb_msg_has(msg, field) == false
, which is an error according to upb.There are two possible fixes for this bug. A guiding principle is that we want the generated C code in wkt.inc to have the same behavior as PHP generated code. Following this principle, the two possible fixes are:
Change the code generator for wkt.inc to check
upb_msg_has(f)
before callingMessage_get()
. This would match the isset() check that the The PHP generated code does, and we would leave the PHP code unchanged.Change
Message_get()
to itself perform theupb_msg_has(f)
check for sub-message fields. This means that generated code would no longer need to perform anisset()
check, so we would want to remove this check from the PHP generated code also to avoid a redundant check.Both of these are reasonable fixes, and it is not immediately obvious which is better. (1) has the benefit of resolving this case when we are in more specialized code (a getter function that already knows this is a sub-message field), and therefore avoids performing the check later in more generic code that would have to test the type again. On the other hand, the
isset()
check is not needed for the pure PHP implementation, as an unset PHP variable will returnnull
anyway. And for the C extension, we'd rather checkupb_msg_has()
at the C level instead of PHP for performance reasons.So this change implements (2). The generated code in wkt.inc remains unchanged, and the PHP generated code for sub-message fields is changed to remove the isset() check.
Fixes: #8659