Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed SEGV in sub-message getters for well-known types when message is unset. #8670

Merged
merged 1 commit into from May 27, 2021

Conversation

@haberman
Copy link
Contributor

@haberman haberman commented May 27, 2021

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:

// PHP generated getter: checks isset().
public function getOptions()
{
    return isset($this->options) ? $this->options : null;
}
// C generated getter, does not check upb_msg_has()
static PHP_METHOD(google_protobuf_Value, getListValue) {
  Message* intern = (Message*)Z_OBJ_P(getThis());
  const upb_fielddef *f = upb_msgdef_ntofz(intern->desc->msgdef,
                                           "list_value");
  zval ret;
  Message_get(intern, f, &ret);
  RETURN_COPY_VALUE(&ret);
}

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 when upb_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:

  1. Change the code generator for wkt.inc to check upb_msg_has(f) before calling Message_get(). This would match the isset() check that the The PHP generated code does, and we would leave the PHP code unchanged.

  2. Change Message_get() to itself perform the upb_msg_has(f) check for sub-message fields. This means that generated code would no longer need to perform an isset() 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 return null anyway. And for the C extension, we'd rather check upb_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

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:

```php
// PHP generated getter: checks isset().
public function getOptions()
{
    return isset($this->options) ? $this->options : null;
}
```

```c
// C generated getter, does not check upb_msg_has()
static PHP_METHOD(google_protobuf_Value, getListValue) {
  Message* intern = (Message*)Z_OBJ_P(getThis());
  const upb_fielddef *f = upb_msgdef_ntofz(intern->desc->msgdef,
                                           "list_value");
  zval ret;
  Message_get(intern, f, &ret);
  RETURN_COPY_VALUE(&ret);
}
```

This led to an error where we wnuld try to get a sub-message field from upb
when it `upb_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:

1. Change the code generator for wkt.inc to check upb_msg_has(f) before
   calling Message_get(). This would match the isset() check that the
   The PHP generated code does, and we would leave the PHP code unchanged.

2. Change Message_get() to itself perform the upb_msg_has(f) check for
   sub-message fields. This means that generated code would no longer need
   to perform an isset() 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
return `null` anyway. And for the C extension, we'd rather check upb_msg_has()
at the C level instead of PHP.

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.
@fowles
fowles approved these changes May 27, 2021
@haberman haberman merged commit b42f237 into protocolbuffers:3.17.x May 27, 2021
53 of 54 checks passed
53 of 54 checks passed
@github-actions
Check for spelling errors
Details
@protobuf-kokoro
Linux Java JDK 7 Kokoro build failed
Details
@protobuf-kokoro
Bazel Kokoro build successful
Details
@protobuf-kokoro
Dist artifact installation Kokoro build successful
Details
@protobuf-kokoro
Linux 32-bit Kokoro build successful
Details
@protobuf-kokoro
Linux C# Kokoro build successful
Details
@protobuf-kokoro
Linux C++ Distcheck Kokoro build successful
Details
@protobuf-kokoro
Linux C++ TC Malloc Kokoro build successful
Details
@protobuf-kokoro
Linux Java Linkage Monitor Kokoro build successful
Details
@protobuf-kokoro
Linux Java Oracle 7 Kokoro build successful
Details
@protobuf-kokoro
Linux JavaScript Kokoro build successful
Details
@protobuf-kokoro
Linux PHP Kokoro build successful
Details
@protobuf-kokoro
Linux Python 2.7 Kokoro build successful
Details
@protobuf-kokoro
Linux Python 2.7 C++ Kokoro build successful
Details
@protobuf-kokoro
Linux Python 3.5 Kokoro build successful
Details
@protobuf-kokoro
Linux Python 3.5 C++ Kokoro build successful
Details
@protobuf-kokoro
Linux Python 3.6 Kokoro build successful
Details
@protobuf-kokoro
Linux Python 3.6 C++ Kokoro build successful
Details
@protobuf-kokoro
Linux Python 3.7 Kokoro build successful
Details
@protobuf-kokoro
Linux Python 3.7 C++ Kokoro build successful
Details
@protobuf-kokoro
Linux Python 3.8 Kokoro build successful
Details
@protobuf-kokoro
Linux Python 3.8 C++ Kokoro build successful
Details
@protobuf-kokoro
Linux Python 3.9 Kokoro build successful
Details
@protobuf-kokoro
Linux Python 3.9 C++ Kokoro build successful
Details
@protobuf-kokoro
Linux Python Release Kokoro build successful
Details
@protobuf-kokoro
Linux Ruby 2.4 Kokoro build successful
Details
@protobuf-kokoro
Linux Ruby 2.5 Kokoro build successful
Details
@protobuf-kokoro
Linux Ruby 2.6 Kokoro build successful
Details
@protobuf-kokoro
Linux Ruby 2.7 Kokoro build successful
Details
@protobuf-kokoro
Linux Ruby 3.0 Kokoro build successful
Details
@protobuf-kokoro
Linux Ruby Release Kokoro build successful
Details
@protobuf-kokoro
MacOS C++ Kokoro build successful
Details
@protobuf-kokoro
MacOS C++ Distcheck Kokoro build successful
Details
@protobuf-kokoro
MacOS JavaScript Kokoro build successful
Details
@protobuf-kokoro
MacOS Obj-C CocoaPods Integration Kokoro build successful
Details
@protobuf-kokoro
MacOS Obj-C OS X Kokoro build successful
Details
@protobuf-kokoro
MacOS Obj-C iOS Debug Kokoro build successful
Details
@protobuf-kokoro
MacOS Obj-C iOS Release Kokoro build successful
Details
@protobuf-kokoro
MacOS PHP7.0 Kokoro build successful
Details
@protobuf-kokoro
MacOS PHP7.3 Kokoro build successful
Details
@protobuf-kokoro
MacOS Python Kokoro build successful
Details
@protobuf-kokoro
MacOS Python C++ Kokoro build successful
Details
@protobuf-kokoro
MacOS Python Release Kokoro build successful
Details
@protobuf-kokoro
MacOS Ruby 2.4 Kokoro build successful
Details
@protobuf-kokoro
MacOS Ruby 2.5 Kokoro build successful
Details
@protobuf-kokoro
MacOS Ruby 2.6 Kokoro build successful
Details
@protobuf-kokoro
MacOS Ruby 2.7 Kokoro build successful
Details
@protobuf-kokoro
MacOS Ruby 3.0 Kokoro build successful
Details
@protobuf-kokoro
MacOS Ruby Release Kokoro build successful
Details
@mergeable
Mergeable Mergeable Run has been Completed!
Details
@protobuf-kokoro
Windows C# Kokoro build successful
Details
@protobuf-kokoro
Windows Csharp Release Kokoro build successful
Details
@protobuf-kokoro
Windows Python Release Kokoro build successful
Details
@google-cla
cla/google All necessary CLAs are signed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants