Skip to content

Conversation

nikita2206
Copy link
Contributor

@nikita2206 nikita2206 commented Apr 19, 2016

fix RECV opcode to handle exceptions thrown from user-defined error handler
as a result of Notice error from failed type coercion

@nikita2206
Copy link
Contributor Author

nikita2206 commented Apr 19, 2016

It might be that RECV_INIT needs this check too, but I'm not sure... http://lxr.php.net/xref/PHP_MASTER/Zend/zend_vm_def.h#4710

Edit: yeah it needed a change, I made it but I'm not sure about EG(exception) check (is it exhaustive enough?)

@@ -4675,7 +4675,7 @@ ZEND_VM_HANDLER(63, ZEND_RECV, NUM, ANY)
}
}

ZEND_VM_NEXT_OPCODE();
ZEND_VM_NEXT_OPCODE_CHECK_EXCEPTION();
Copy link
Member

Choose a reason for hiding this comment

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

This should be handled the same way as recv_init, i.e. by checking eg(exception) in the verify_arg_type branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, thanks for review

@laruence
Copy link
Member

please also wrap EG(exception) checking into UNEXCEPT , and could you please squash these commits into one? I need cherry-pick them down to PHP-7.0, thanks

@laruence laruence added the Bug label Apr 20, 2016
fix RECV opcode to handle exceptions thrown from user-defined error handler
as a result Notice error from failed type coercion
@nikita2206 nikita2206 force-pushed the fix-recv-opcode-handle-exception branch from bc22b23 to ed9a1a9 Compare April 20, 2016 08:57
@nikita2206
Copy link
Contributor Author

@laruence squashed and rebased it on PHP-7.0 but github can't change PR target branch. The check was already in UNEXPECTED, too many parenthesis make it hard to distinguish :)

@php-pulls php-pulls merged commit ed9a1a9 into php:master Apr 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants