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

Assertion `(key)->h != 0 && "Hash must be known"' failed. #10570

Closed
Changochen opened this issue Feb 11, 2023 · 2 comments
Closed

Assertion `(key)->h != 0 && "Hash must be known"' failed. #10570

Changochen opened this issue Feb 11, 2023 · 2 comments

Comments

@Changochen
Copy link

Description

The following code:

<?php
$$P = new stdClass();
for (; ; ) {
    $$a->{90};
    $$a->{0} = 0;
}
?>

Resulted in this output:

php-src/Zend/zend_hash.c:673: zend_hash_find_bucket: Assertion `(key)->h != 0 && "Hash must be known"' failed.

PHP Version

PHP 8.3.0-dev

Operating System

No response

@nielsdos
Copy link
Member

Can confirm on >=8.1. Simpler reproducer (without variable-variables):

<?php
$a = new stdClass();
for (; ; ) {
    $a->{90};
    $a->{0} = 0;
}

@nielsdos
Copy link
Member

This is what happens:
The problem is the $a->{90} line. At the first iteration there is no cache slot set yet, so the following code is executed:

name = Z_STR_P(GET_OP2_ZVAL_PTR(BP_VAR_R));

Note that the hash is not computed here.

Then in the second iteration there is a cache slot, but still the hash of name is not computed yet. The lookup in the zobj->properties table is done with zend_hash_find_known_hash which assumes name has a hash and therefore it crashes with an assertion failure.

While changing zend_hash_find_known_hash to zend_hash_find would fix this particular issue, I don't think that's entirely right because the hash is already used prior to that function call:

(EXPECTED(p->h == ZSTR_H(name)) &&

While we could add a zend_string_hash_val at the start of that branch, that would slow down the case where we do have a hash and a cache slot already because of the extra check. So I propose to add a zend_string_hash_val when the name string is assigned the first time instead.

nielsdos added a commit to nielsdos/php-src that referenced this issue Feb 12, 2023
nielsdos added a commit to nielsdos/php-src that referenced this issue Feb 12, 2023
Co-authored-by: Changochen <changochen1@gmail.com>
nielsdos added a commit to nielsdos/php-src that referenced this issue Feb 12, 2023
…iled.

Fixes phpGH-10570, see phpGH-10570 for analysis.

Co-authored-by: Ilija Tovilo <ilija.tovilo@me.com>
nielsdos added a commit to nielsdos/php-src that referenced this issue Feb 12, 2023
Co-authored-by: Changochen <changochen1@gmail.com>
iluuu1994 added a commit that referenced this issue Feb 24, 2023
* PHP-8.1:
  Fix GH-10570: Assertion `(key)->h != 0 && "Hash must be known"' failed.
iluuu1994 added a commit that referenced this issue Feb 24, 2023
* PHP-8.2:
  Fix GH-10570: Assertion `(key)->h != 0 && "Hash must be known"' failed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants
@nielsdos @Changochen and others