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

Seg fault when using an object returned directly via a method #6

Closed
beest opened this issue Mar 20, 2013 · 4 comments
Closed

Seg fault when using an object returned directly via a method #6

beest opened this issue Mar 20, 2013 · 4 comments

Comments

@beest
Copy link
Collaborator

beest commented Mar 20, 2013

The following code sample is the minimum required to reproduce the problem. As you can see, the problem occurs via __get and via an existing method.

I'm testing with PHP-5.3.10 and libv8.so version 3.17.13.

<?php

class Service
{
    public function method()
    {
        print("method\n");
    }
}

class Container
{
    private $service = null;

    public function __get($name)
    {
        if ($name == 'crash') {
            return new Service;
        } else {
            $this->service = new Service;
            return $this->service;
        }
    }

    public function getService()
    {
        return new Service;
    }
}

$v8Js = new V8Js('PHP');
$v8Js->container = new Container;

$code = <<<EOT

var container = PHP.container;

// This works OK
//container.ok.method();

// These both cause seg fault
container.crash.method();
container.getService().method();

EOT;

$v8Js->executeString($code, 'test');

The seg fault happens during the zend_get_class_entry (via Z_OBJCE_P) call in the php_v8js_php_callback function (line 46 in v8js_convert.cc).

I have limited knowledge of Zend internals, but this looks like an object referencing issue. The pointer that is passed into Z_OBJCE_P is coming via the SetAlignedPointerInInternalField. Perhaps we need to explicitly add a reference to the object and use that in the SetAlignedPointerInInternalField call instead?

I'm willing to help fix this if anybody can provide any insights into why this might be happening.

@beest
Copy link
Collaborator Author

beest commented Mar 20, 2013

With my limited knowledge, I have been experimenting with this issue and I've managed to prevent the seg fault by forcefully increasing the refcount of the zval.

I've done this with a call to Z_SET_REFCOUNT just before calling SetAlignedPointerInInternalField, and only in the case where the refcount is 1.

...
if (Z_REFCOUNT_P(value) == 1)
{
    Z_SET_REFCOUNT_P(value, 2);
}

newobj->SetAlignedPointerInInternalField(0, (void *) value);
...

I appreciate this might be a horrible hack, but it hopefully sheds some light on a fix for this issue.

@beest
Copy link
Collaborator Author

beest commented Mar 22, 2013

I've thought about this more, and the above fix might actually be close to the solution that is needed.

The root cause of the bug is when an object is instantiated and returned without being stored in any other variable in PHP land. The refcount of the object will decrease to zero, and the garbage collection will remove the object from memory. This is what causes the seg fault when accessing the object via GetAlignedPointerInInternalField.

In fact, the fix should probably always increase the refcount. Not doing so would potentially cause deeper issues later on when variables in PHP are freed but Javascript references still need to be in memory.

@beest
Copy link
Collaborator Author

beest commented Mar 22, 2013

So the final fix would be:

Z_SET_REFCOUNT_P(value, Z_REFCOUNT_P(value) + 1);

There may be a macro that does this more elegantly (something like Z_INC_REFCOUNT_P), but I'm not sure what it is.

@satoshi75nakamoto
Copy link
Contributor

Yeah, that makes sense. I'll get a patch for it ready in the next couple days. Thanks, @beest.

beest pushed a commit to beest/v8js that referenced this issue Nov 21, 2013
… horrible hack but it works for now until we have a better solution.
ABaldwinHunter pushed a commit to ABaldwinHunter/v8js-clone that referenced this issue Feb 2, 2016
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