Skip to content

Commit

Permalink
Fix bug #79599 in a different way
Browse files Browse the repository at this point in the history 
Move the emission of the undefined variable notice before the
array separation.
  • Loading branch information
@nikic
nikic committed Jul 9, 2020
1 parent a3cb612 commit 5795dfd
Show file tree
Hide file tree
Showing 3 changed files with 77 additions and 116 deletions.
27 changes: 27 additions & 0 deletions Zend/tests/bug79599.phpt
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
--TEST--
Bug #79599 (coredump in set_error_handler)
--FILE--
<?php
set_error_handler(function($code, $message){
throw new \Exception($message);
});
function test1(){
$a[] = $b;
}
function test2(){
$a[$c] = $b;
}
try{
test1();
}catch(\Exception $e){
var_dump($e->getMessage());
}
try{
test2();
}catch(\Exception $e){
var_dump($e->getMessage());
}
?>
--EXPECT--
string(21) "Undefined variable: b"
string(21) "Undefined variable: b"
6 changes: 2 additions & 4 deletions Zend/zend_vm_def.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2619,15 +2619,14 @@ ZEND_VM_HANDLER(23, ZEND_ASSIGN_DIM, VAR|CV, CONST|TMPVAR|UNUSED|NEXT|CV, SPEC(O

if (EXPECTED(Z_TYPE_P(object_ptr) == IS_ARRAY)) {
ZEND_VM_C_LABEL(try_assign_dim_array):
value = GET_OP_DATA_ZVAL_PTR(BP_VAR_R);
SEPARATE_ARRAY(object_ptr);
if (OP2_TYPE == IS_UNUSED) {
value = GET_OP_DATA_ZVAL_PTR(BP_VAR_R);
if (OP_DATA_TYPE == IS_CV || OP_DATA_TYPE == IS_VAR) {
ZVAL_DEREF(value);
}
variable_ptr = zend_hash_next_index_insert(Z_ARRVAL_P(object_ptr), value);
if (UNEXPECTED(variable_ptr == NULL)) {
FREE_OP_DATA();
zend_cannot_add_element();
ZEND_VM_C_GOTO(assign_dim_error);
} else if (OP_DATA_TYPE == IS_CV) {
Expand Down Expand Up @@ -2656,7 +2655,6 @@ ZEND_VM_C_LABEL(try_assign_dim_array):
if (UNEXPECTED(variable_ptr == NULL)) {
ZEND_VM_C_GOTO(assign_dim_error);
}
value = GET_OP_DATA_ZVAL_PTR(BP_VAR_R);
value = zend_assign_to_variable(variable_ptr, value, OP_DATA_TYPE, EX_USES_STRICT_TYPES());
}
if (UNEXPECTED(RETURN_VALUE_USED(opline))) {
Expand Down Expand Up @@ -2707,7 +2705,7 @@ ZEND_VM_C_LABEL(try_assign_dim_array):
}
dim = GET_OP2_ZVAL_PTR(BP_VAR_R);
ZEND_VM_C_LABEL(assign_dim_error):
FREE_UNFETCHED_OP_DATA();
FREE_OP_DATA();
if (UNEXPECTED(RETURN_VALUE_USED(opline))) {
ZVAL_NULL(EX_VAR(opline->result.var));
}
Expand Down

6 comments on commit 5795dfd

@laruence
Copy link
Member

Choose a reason for hiding this comment

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

I dont’ think this fix is complete,
and actually this was the first way I thought too, but this could not fix situation likes:

<?php
$a = NULL;
set_error_handler(function($code, $message, $arg, $in, $context){
	global $a;
	try {
		throw new \Exception($message);	
	} catch (Exception $e) {
		$a = $e->getTrace();
	}
});
function test1(){
	$a[$c] = 1;
}
test1();
?>

@nikic
Copy link
Member Author

@nikic nikic commented on 5795dfd Jul 9, 2020

Choose a reason for hiding this comment

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

@laruence Yes, I'm aware of this issue. There are many problems with notices being thrown during write operations. I've recently fixed some of them, but it's really hard to make code safe against it and I think the approach does not scale. We discussed this with @bwoebi yesterday, and he suggested that we should introduce a deferred notice mechanism, so that notices will only be thrown after the opcode. This may have slightly different behavior when it comes to side-effects, but it would avoid most of these dangerous situations. This is something we may want to try in master.

@laruence
Copy link
Member

@laruence laruence commented on 5795dfd Jul 9, 2020 •
edited

Choose a reason for hiding this comment

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

Always deep duplicating the symbol table should fix this kind of problems at all, but you reverted it... :<

@nikic
Copy link
Member Author

@nikic nikic commented on 5795dfd Jul 9, 2020

Choose a reason for hiding this comment

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

@laruence Duplicating the symbol table only removes one vector for accessing the variable. Unfortunately there are many others, such as $GLOBALS and references.

@laruence
Copy link
Member

Choose a reason for hiding this comment

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

I mean kind of problems means “the exception’s trace issue”, others are not the same..
anyway, please take care of the case I pasted above, since it should be broken now. thanks

@nikic
Copy link
Member Author

@nikic nikic commented on 5795dfd Jul 9, 2020

Choose a reason for hiding this comment

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

@laruence I understand, but I don't see a reason why we should address the backtrace issue without addressing the more general issue. Your patch fixed the problem for

<?php
set_error_handler(function($code, $message, $arg, $in, $context){
    $GLOBALS['b'] = $context['a'];
});
$a[$c] = 1;

But did not fix the problem for the nearly identical case of:

<?php
set_error_handler(function($code, $message, $arg, $in, $context){
    $GLOBALS['b'] = $GLOBALS['a'];
});
$a[$c] = 1;

A partial fix is of course also good, but in this case the partial fix caused huge performance regressions.

The only case that's really important to handle is the throw new Exception one, because this is a very common pattern. This case needs to work for sure (and does work). For other cases, I would like to have a general solution with reasonable performance impact.

Please sign in to comment.