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

PHP Extension - Segmentation fault if message constructor is not called. #19978

Open
FabioBatSilva opened this issue Jan 13, 2025 · 5 comments
Open
Assignees

Comments

@FabioBatSilva
Copy link

FabioBatSilva commented Jan 13, 2025

We are trying to migrate an application from the PHP Protobuf implementation to the C extension.

A few tests are using the prophecy to mock objects during tests.
and prophecy ends up creating a proxy that looks something like this :

class P5
    extends \UserProfile\V1alpha\Account
    implements \Prophecy\Doubler\DoubleInterface, \Prophecy\Prophecy\ProphecySubjectInterface, \Prophecy\Doubler\Generator\ReflectionInterface
{
    private $objectProphecyClosure;

    public  function __construct( $data = NULL) {
        if (0 < func_num_args()) {
            call_user_func_array(array('parent', '__construct'), func_get_args());
        }
    }

    public  function setProphecy(\Prophecy\Prophecy\ProphecyInterface $prophecy) {
        if (null === $this->objectProphecyClosure) {
            $this->objectProphecyClosure = static function () use ($prophecy) {
                return $prophecy;
            };
        }
    }

    public  function getProphecy() {
        return \call_user_func($this->objectProphecyClosure);
    }
}

Since the parent constructor is not always called it seems the message never properly initialized and the field lookup fails when get_field is called.

NOTE: This is a common pattern in many php frameworks.
And it is debatable weather or not this should be mocked/proxied but the behaviour is inconsistent between the two implementations and I believe no condition should trigger a seg fault.


What version of protobuf and what language are you using?
Version: main
Language: php
OS: Linux

What did you do?

<?php
# proxy_test.php

require_once('../vendor/autoload.php');
require_once('test_util.php');

use Foo\TestMessage;

class TestMessageProxy extends TestMessage
{
    private $foo;

    public  function __construct($data = NULL) {
        if (0 < func_num_args()) {
            parent::__construct($data);
        }
    }

    public function setFoo($foo) {
        $this->foo = $foo;
    }

    public function getFoo() {
        return $this->foo;
    }
}


$p = new TestMessageProxy();

$p->setFoo('bar');

assert('bar' === $p->getFoo());
# Compile the extension 
./php/tests/compile_extension.sh

# Run script
php -d display_errors=on -dextension=../ext/google/protobuf/modules/protobuf.so proxy_test.php

What did you expect to see

No error

What did you see instead?

Segmentation fault (core dumped)

gdb

in get_field (msg=0x7ffff2bfb6c0, msg=0x7ffff2bfb6c0, member=<optimized out>)
    at /tmp/pear/temp/protobuf/message.c:92

bt
#0  0x00007ffff3fb7836 in get_field (msg=0x7ffff280cd20, msg=0x7ffff280cd20, member=<optimized out>)
    at /tmp/pear/temp/protobuf/message.c:92
#1  Message_read_property (obj=0x7ffff280cd20, member=0x7ffff3705a80, type=<optimized out>, cache_slot=0x7fffec4e8e98, 
    rv=0x7ffff4817c80) at /tmp/pear/temp/protobuf/message.c:312
#2  0x00005555559dc082 in execute_ex ()
#3  0x000055555563c839 in ?? ()
@FabioBatSilva FabioBatSilva added the untriaged auto added to all issues by default when created. label Jan 13, 2025
@FabioBatSilva FabioBatSilva changed the title PHP Extension Segmentation fault if constructor is not called. PHP Extension Segmentation fault if message constructor is not called. Jan 13, 2025
@esrauchg
Copy link

Can you clarify in your TestMessageProxy repro, how you are reaching get_field?

In practice if the C get_field is reached and the ctor has not been called, there's no possible way of it actually working; in theory it could defensively check and throw a php exception (though its unfortunate we'd be constantly performing these checks just in case you're in this test case where the ctor was suppressed).

But it seems more likely that TestMessageProxy example should never reach the C code at all?

@FabioBatSilva
Copy link
Author

FabioBatSilva commented Jan 13, 2025

It is triggered by the getter / setter. when the $this->foo expression invokes Message_{write,read,has}_property

I believe with the proxy extending the TestMessage end overriding the constructor the message end up not initialised.
And and when get_field can't find the msg/field definition the extension seg faults.

I think in the PHP implementation the behaviour is different BC that never triggers any code relies on the descriptor / descriptor pool.

But Perhaps the issue here is how the object_handlers callback behaves ?
I think currently the extension assumes all properties are part of the Message object.

But that is not the case any time the message is extended.

Perhaps it needs a fallback.. something like this :

zval* Message_write_property(zend_object* obj, zend_string* member, zval* val, void** cache_slot) {
  const upb_FieldDef* f = try_lookup_field(obj, member);
 
  // If the field belongs to the message, set it.
  if (f) {
    if (Message_set(obj, f, val)) {
      return val;
    }

    return &EG(error_zval); 
  }

  // If not delegate to the standard write_property handler.
  return zend_std_write_property(obj, member, val, cache_slot);
}

See: https://github.com/php/php-src/blob/857ce2c9e00be33c820cc8cebc2a039179f97ee0/ext/mysqli/mysqli.c#L260-L290

@esrauchg
Copy link

Ah, I understand now.

We definitely would not want it to be designed to silently fallback to PHP properties if it can't be stored properly; the point of protobuf is to create objects that will be serialized, and any time that path would be reached outside of exactly this weird mocking case it would likely imply a severe production-impacting bug where the user tried to set something and it silently didn't actually affecting the content that will go over the wire. I think we'd view that as a path where we intentionally made data corruption/data loss be a silently and hard-to-observe behavior.

I would have to think about this a bit, but I think realistically the resolution from our side for this is less likely to actually support the behavior and instead be more like having it consistently throw a PHP exception rather than segfault. Do you feel like this pattern (subclassing and preventing ctor) is actually so common in PHP frameworks that use it would expect it to consistently work, versus have arbitrary behavior?

@FabioBatSilva
Copy link
Author

Yes, I think so.. Likely why other extensions support.
phpspec, phpunit, mockery and other frameworks make extensive use of sub classing to mock / spy / proxy objects.
Preventing construct calls I'm not sure. I've seen a few use cases for serialisation.. But not as common as far as i know..

I don't think proxy is a particularly good pattern to apply on protobuf message but is is definitely used and (unintentionally?) possible with the php protobuf implementation.

If you guys decide no to support extending the messages I'd argue that the generated classes should probably be made final.
That way you don't need to rely on the runtime validations and it is clear that it is not supported.

@esrauchg
Copy link

Thanks for the clarifications. We're discussing what to do about this internally, will get back to this thread with updates when I have it.

@esrauchg esrauchg self-assigned this Jan 15, 2025
@esrauchg esrauchg removed the untriaged auto added to all issues by default when created. label Jan 15, 2025
@FabioBatSilva FabioBatSilva changed the title PHP Extension Segmentation fault if message constructor is not called. PHP Extension - Segmentation fault if message constructor is not called. Jan 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants