Skip to content

Fix oss-fuzz-61469: Undef dynamic property in ++/-- unset in error handler #12011

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

Closed
wants to merge 5 commits into from

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Aug 21, 2023

So this issue is caused by the undefined property warning generated in the standard get_property_ptr_ptr object handler.

I suppose this might have been a pre-existing issue if an extension get_property_ptr_ptr handler did something to the ZVAL but considering what needs to be done this seems unlikely.

The easiest solution is to just consider the value as null and move forward with it.

@dstogov is this approach a good idea or not?

@iluuu1994
Copy link
Member

I'll need to look into this in detail, but when I checked the oss-fuzz report this was an issue for ++, -- (pre and post), += and -= alike.

@Girgias
Copy link
Member Author

Girgias commented Aug 22, 2023

Indeed, I was confused why the pre increment change also worked for post increment. But it seems there is a compile time optimization to rewrite a post increment to a pre one if the return value is not used. (Or a for loop might also leave the post increment)

Confirmed and fixed the binop case

@dstogov
Copy link
Member

dstogov commented Aug 29, 2023

It looks like the problem is already fixed by a3a3964
Anyway, it make sense to commit the tests.

Post increments are converted to pre increments if no return value is used.
Ditto for decrement
@Girgias
Copy link
Member Author

Girgias commented Aug 30, 2023

It looks like the problem is already fixed by a3a3964 Anyway, it make sense to commit the tests.

I'm not sure this is totally fixed, as if I revert the VM changes the following test:

--TEST--
OSS Fuzz #61469: Undef variable in ++/-- for dynamic property that is unset in error handler
--FILE--
<?php
class C {
    function errorHandle() {
        unset($this->a);
    }
}
$c = new C;
set_error_handler([$c,'errorHandle']);
$c->a += 5;
var_dump($c->a);
?>

Results in dumping int(5), same with the pre increment variations. But every other test continues to dump NULL

@Girgias Girgias changed the base branch from master to PHP-8.3 August 30, 2023 19:52
Copy link
Member

@dstogov dstogov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved.

Please, also take a look into oss-fuzz 61865, that looks similar

<?php
class C {
    public $a;
    function errorHandler() {
        unset($this->a);
    }
}
$c=new C;
set_error_handler([$c,'errorHandler']);
array($y);
(++$c->a);

@Girgias Girgias closed this in 013bb57 Sep 2, 2023
@Girgias Girgias deleted the oss-fuzz-inc-dec branch September 2, 2023 22:35
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

Successfully merging this pull request may close these issues.

3 participants