Skip to content

Use-after-free in extract() with EXTR_REFS #18211

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 1 commit into from

Conversation

iluuu1994
Copy link
Member

Fixes GH-18209

@nielsdos
Copy link
Member

nielsdos commented Mar 31, 2025

We should be using some try_assign stuff anyway, otherwise this is still broken:

<?php

class C {
    public function __destruct() {
        var_dump($GLOBALS['b']);
        $GLOBALS['b'] = 43;
    }
}

class D {
    public static C $x;
}

D::$x = new C;
$b =& D::$x;

$array = ['b' => 42];
extract($array, EXTR_REFS);

var_dump($b);
var_dump(D::$x); // 42! type system broke :(

EDIT: NVM as pointed out on Slack (sorry!)

@iluuu1994
Copy link
Member Author

Ugh, I hate references. Relevant, maybe in 10 years we can get rid of them. 🥲

@iluuu1994
Copy link
Member Author

As mentioned privately, the example is actually correct. The first two prints come from the var_dump()s in main, the last one comes from __destruct(). C is only destroyed during shutdown as D::$x holds on to it.

I will change this to use zend_safe_assign_to_variable_noref() on master, as you have suggested in Slack. 🙂

@iluuu1994 iluuu1994 closed this in a21065e Apr 1, 2025
@iluuu1994
Copy link
Member Author

Turns out we can't use zend_safe_assign_to_variable_noref() specifically because semantics are weird, and we're writing over existing references. However, zend_safe_assign_to_variable_noref() asserts that the variable written to is not a reference, assuming any references would have been DEREFed, which is generally the correct thing to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants