Fix GH-13931: Applying zero offset to null pointer in Zend/zend_opcode.c #13938
Conversation
…code.c In the test cases, the compiler bails out due to a fatal error. The data structures used by the compiler will contain stale values. In particular, for the test case CG(loop_var_stack) will contain data. The next compilation will incorrectly use elements from the previous stack. To solve this, we reset part of the compiler data structures. We don't do a full re-initialization via init_compiler() because that will also reset streams and resources.
Zend/zend.c
Outdated
| * 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(); |
There was a problem hiding this comment.
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. 🙂
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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().
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
In the test cases, the compiler bails out due to a fatal error. The data structures used by the compiler will contain stale values. In particular, for the test case CG(loop_var_stack) will contain data. The next compilation will incorrectly use elements from the previous stack. This causes a segfault in pass2 because wrong instructions will be emitted due to the stale data.
To solve this, we reset part of the compiler data structures. We don't do a full re-initialization via init_compiler() because that will also reset streams and resources.