Skip to content

Fix GH-13931: Applying zero offset to null pointer in Zend/zend_opcode.c #13938

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.

Sign up for GitHub

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 6 commits into from
Closed

Fix GH-13931: Applying zero offset to null pointer in Zend/zend_opcode.c #13938

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fd6e7f9
Fix GH-13931: Applying zero offset to null pointer in Zend/zend_opcode.c
ndossche Apr 10, 2024
0615e00
Add tests
ndossche Apr 10, 2024
5a61065
Move code to php_error_cb
ndossche Apr 11, 2024
9a85794
Bailout already sets in_compilation=0
ndossche Apr 12, 2024
e7e2160
Only recover the compiler on compilation or parse error
ndossche Apr 12, 2024
438ffdb
Fix indentation
ndossche Apr 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions Zend/tests/gh13931.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
--TEST--
GH-13931 (Applying zero offset to null pointer in Zend/zend_opcode.c)
--FILE--
<?php

register_shutdown_function(function() {
var_dump(eval("return 1+3;"));
});

eval(<<<EVAL
function foo () {
try {
break;
} finally {
}
}
foo();
EVAL);

?>
--EXPECTF--
Fatal error: 'break' not in the 'loop' or 'switch' context in %s on line %d
int(4)
12 changes: 11 additions & 1 deletion Zend/zend.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1176,9 +1176,19 @@ ZEND_API ZEND_COLD ZEND_NORETURN void _zend_bailout(const char *filename, uint32
exit(-1);
}
gc_protect(1);

if (CG(in_compilation)) {
CG(in_compilation) = 0;
/* We bailout during compilation which may for example leave stale entries in CG(loop_var_stack).
* If code is compiled during shutdown, we need to make sure the compiler is reset to a clean state,
* otherwise this will lead to incorrect compilation during shutdown.
* We don't do a full re-initialization via init_compiler() because that will also reset streams and resources. */
shutdown_compiler();
Copy link
Member

Choose a reason for hiding this comment

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

Other calls to shutdown_compiler are wrapped in zend_try, do we need that here? I don't believe so, looking at its implementation. But maybe you can double-check. 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't really know why other locations surround it in a try, it doesn't seem necessary.
However, if wanted I can do this, it doesn't seem harmful.

Copy link
Member

Choose a reason for hiding this comment

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

I checked again, and none of the frees in shutdown_compiler can throw, because they are either just calls to efree, or free hash tables that contain strings or pointers.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if we call shutdown_function after a fatal error on purpose. This is definitely not safe.
We especially disable destructor calls in php_error_cb().

In any case, I think it's better to move this recovery code into php_error_cb().

Copy link
Member Author

Choose a reason for hiding this comment

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

I have moved the code to php_error_cb.

I'm not sure if we call shutdown_function after a fatal error on purpose. This is definitely not safe.
We especially disable destructor calls in php_error_cb().

Good question, I don't know but I wouldn't be surprised people depend on this.

Copy link
Member

Choose a reason for hiding this comment

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

@dstogov Can you clarify why this would be problematic for shutdown functions? When compiling new files through includes in shutdown, the compile structures are cleaned and newly initialized. Nothing should be referencing the old values either, as the compilation has previously bailed. I might be missing something.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if we call shutdown_function after a fatal error on purpose. This is definitely not safe.
We especially disable destructor calls in php_error_cb().

Good question, I don't know but I wouldn't be surprised people depend on this.

This is right. People depends on every weird feature :(
This error handler may be called after ANY fatal error that stood something in inconsistent state. E.g. after memory overflow.

Copy link
Member

Choose a reason for hiding this comment

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

@dstogov Can you clarify why this would be problematic for shutdown functions? When compiling new files through includes in shutdown, the compile structures are cleaned and newly initialized. Nothing should be referencing the old values either, as the compilation has previously bailed. I might be missing something.

I don't claim your recovery code. Recovery after parse errors should be possible.
I ask you to move this code to a more appropriate place (into php_error_cb(). or even higher).
Also it may make sense to do this only for E_PARSE errors.

Copy link
Member Author

Choose a reason for hiding this comment

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

The error from the test case is an E_COMPILE_ERROR instead of a E_PARSE.
I adjusted the code in php_error_cb to recover the compiler if either of these two happen.

zend_init_compiler_data_structures();
}

CG(unclean_shutdown) = 1;
CG(active_class_entry) = NULL;
CG(in_compilation) = 0;
CG(memoize_mode) = 0;
EG(current_execute_data) = NULL;
LONGJMP(*EG(bailout), FAILURE);
Expand Down
21 changes: 21 additions & 0 deletions sapi/phpdbg/tests/gh13931.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
--TEST--
Applying zero offset to null pointer in Zend/zend_opcode.c
--FILE--
<?php
function foo () {
try {
break;
} finally {
}
}
foo();
?>
--PHPDBG--
ev 1 + 3
ev 2 ** 3
q
--EXPECTF--
Fatal error: 'break' not in the 'loop' or 'switch' context in %s on line %d
prompt> 4
prompt> 8
prompt>