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

[nielsdos]

Fixes #10570, see #10570 for analysis.

[iluuu1994]

Tested locally and apparently my assumption is incorrect. I think it would be better to make sure the hash is there for constant values.

[nielsdos]

I think it would be better to make sure the hash is there for constant values.

Isn't that wasteful for performance? Forcing the computation of the hash for every string constant in the AST degrades performance for the cases where we don't need the hash. It seems better to me to only compute the hash if it is actually needed, like how this PR fixes the issue?

[iluuu1994]

@nielsdos The downside to this is that it checks if the hash exists for all property accesses, which is very very rarely the necessary. Generally, we care more about runtime performance than compile time performance.

[iluuu1994]

diff --git a/Zend/zend_compile.c b/Zend/zend_compile.c
index 9ca8c654fc..0774fb6d19 100644
--- a/Zend/zend_compile.c
+++ b/Zend/zend_compile.c
@@ -2924,6 +2924,7 @@ static zend_op *zend_delayed_compile_prop(znode *result, zend_ast *ast, uint32_t
 	opline = zend_delayed_emit_op(result, ZEND_FETCH_OBJ_R, &obj_node, &prop_node);
 	if (opline->op2_type == IS_CONST) {
 		convert_to_string(CT_CONSTANT(opline->op2));
+		zend_string_hash_val(Z_STR_P(CT_CONSTANT(opline->op2)));
 		opline->extended_value = zend_alloc_cache_slots(3);
 	}
 
diff --git a/Zend/zend_vm_def.h b/Zend/zend_vm_def.h
index 6045eee904..ea9fdb1472 100644
--- a/Zend/zend_vm_def.h
+++ b/Zend/zend_vm_def.h
@@ -2111,7 +2111,7 @@ ZEND_VM_C_LABEL(fetch_obj_r_fast_copy):
 				}
 			}
 			name = Z_STR_P(GET_OP2_ZVAL_PTR(BP_VAR_R));
-			zend_string_hash_val(name);
 		} else {
 			name = zval_try_get_tmp_string(GET_OP2_ZVAL_PTR(BP_VAR_R), &tmp_name);
 			if (UNEXPECTED(!name)) {
@@ -2269,7 +2269,7 @@ ZEND_VM_C_LABEL(fetch_obj_is_fast_copy):
 				}
 			}
 			name = Z_STR_P(GET_OP2_ZVAL_PTR(BP_VAR_R));
-			zend_string_hash_val(name);
 		} else {
 			name = zval_try_get_tmp_string(GET_OP2_ZVAL_PTR(BP_VAR_R), &tmp_name);
 			if (UNEXPECTED(!name)) {

I think that should do it.

[nielsdos]

Okay. I'll squash your changes into my commit and add your C-a-b tag soon. Thank you.

[iluuu1994]

As for the non-const case, this would need a regression test. Could you try something like this?

$a = 0;
$b = 0;
$key = $a + $b; 
$o = new stdClass();
$o->{$key} = 0;

Does this currently fail?

[nielsdos]

That field name would be "0" which is an interned string so the hash is precomputed. You'd need something like $a=$b=10 such that we don't hit an interned string. But even then it doesn't fail because in zend_std_read_property we'll either compute the hash in the zend_get_property_offset call, or we hit the IS_DYNAMIC_PROPERTY_OFFSET case which also seems to compute the hash.

I did manage to create a testcase which does not trigger a hash computation:

class MyStringable1 {
    public function __toString(): string {
        return str_repeat("a", 3);
    }
}

$field1 = new MyStringable1();

$test = new stdClass();
for ($i = 0; $i < 2; $i++) {
    var_dump($test->{$field1});
}

but since a cache slot won't be used here I don't think this can cause problems... So the zend_string_hash_val is probably unnecessary in the non-const path.

[iluuu1994]

Right, you'd need a larger int because 0-9 are available as interned strings as an optimization.

Edit: But if the error occurs when handling cache slots this shouldn't be an issue either, as this would be a CV.

[nielsdos]

Okay, so I'll remove the zend_string_hash_val lines I added in the VM, such that only the zend_compile.c file has an extra zend_string_hash_val. And I'll not add a test for the non-const case as it can't cause problems.
And I'll add your co-authored-by tag, although it's more like an "authored-by" tag since you just solved all the issues :p So it doesn't really feel right for me to take any credit for this

[iluuu1994]

@nielsdos Don't worry about it, the bigger effort is tracking down the issue and making the changes, so thank you for taking the time to do so!

[nielsdos]

@iluuu1994 Updated the commits, thanks.

[iluuu1994]

Perfect, thank you! Let's leave this open for a day or two to see if anybody else has feedback.

[Girgias]

Makes sense to me. :)

[iluuu1994]

Forgot about this one. Merged, thank you 🙂