Skip to content

Fix GH-10709: Recursive class constant evaluation #10712

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Fix GH-10709: Recursive class constant evaluation #10712

wants to merge 1 commit into from

Conversation

iluuu1994
Copy link
Member

Fixes https://oss-fuzz.com/testcase-detail/6445949468934144

We currently make no distinction in zval_update_constant_ex() regarding what type of constant is updated. This can include constants, class constants, default properties, enums, amongst others. Most of those are triggered implicitly (e.g. by instantiating a class) but some are loaded explicitly (e.g. accessing a specific constant). Updating the constants can trigger the autoloader and thus userland code. If this userland code explicitly triggers an evaluation of the AST that has triggered the autoloader in the first place, this can lead to two nested calls of the same AST evaluation. When the nested one finishes it will destroy the AST, thus leading to a use-after-free in the outer one. To avoid this, guard for nested class const evaluations in a new zend_update_class_constant() function.

I have not yet definitively checked if any other AST evaluations can lead to similar issues. I will do so soon. I'll also look at the fixme in reflection.c.

/cc @nielsdos

@iluuu1994 iluuu1994 requested a review from dstogov February 26, 2023 14:28
@dstogov
Copy link
Member

dstogov commented Feb 27, 2023

I didn't get why do we need this separation of new function. Please explain.

Zend/tests/gh10709.phpt seems to work fine without the patch.
The behaviour of few existing tests is changed.
I don't like this, especially for PHP-8.1.

@iluuu1994
Copy link
Member Author

@dstogov Did you run the test with -fsanitize=address? Valgrind also seems to catch it. The issue is not easy to encounter in real applications so I can retarget this to master.

Here's the ASAN output 

string(23) "this is A extended by B"
=================================================================
==587089==ERROR: AddressSanitizer: heap-use-after-free on address 0x60c000008998 at pc 0x000001a961e2 bp 0x7ffce2c99f70 sp 0x7ffce2c99f68
READ of size 8 at 0x60c000008998 thread T0
    #0 0x1a961e1 in zend_ast_evaluate_inner /home/ilutov/Developer/php-src/Zend/zend_ast.c:546
    #1 0x1a95d63 in zend_ast_evaluate_ex /home/ilutov/Developer/php-src/Zend/zend_ast.c:522
    #2 0x15c8f62 in zval_update_constant_with_ctx /home/ilutov/Developer/php-src/Zend/zend_execute_API.c:696
    #3 0x15c92ba in zval_update_constant_ex /home/ilutov/Developer/php-src/Zend/zend_execute_API.c:710
    #4 0x17a29c6 in ZEND_FETCH_CLASS_CONSTANT_SPEC_CONST_CONST_HANDLER /home/ilutov/Developer/php-src/Zend/zend_vm_execute.h:7243
    #5 0x19de180 in execute_ex /home/ilutov/Developer/php-src/Zend/zend_vm_execute.h:57172
    #6 0x15cd115 in zend_call_function /home/ilutov/Developer/php-src/Zend/zend_execute_API.c:938
    #7 0x15cea26 in zend_call_known_function /home/ilutov/Developer/php-src/Zend/zend_execute_API.c:1032
    #8 0xf557b2 in spl_perform_autoload /home/ilutov/Developer/php-src/ext/spl/php_spl.c:448
    #9 0x15d0943 in zend_lookup_class_ex /home/ilutov/Developer/php-src/Zend/zend_execute_API.c:1198
    #10 0x15d3041 in zend_fetch_class_by_name /home/ilutov/Developer/php-src/Zend/zend_execute_API.c:1664
    #11 0x17c99dd in ZEND_NEW_SPEC_CONST_UNUSED_HANDLER /home/ilutov/Developer/php-src/Zend/zend_vm_execute.h:10454
    #12 0x19dfe1e in execute_ex /home/ilutov/Developer/php-src/Zend/zend_vm_execute.h:57468
    #13 0x19f4e41 in zend_execute /home/ilutov/Developer/php-src/Zend/zend_vm_execute.h:60935
    #14 0x163aa29 in zend_execute_scripts /home/ilutov/Developer/php-src/Zend/zend.c:1798
    #15 0x13cfd7e in php_execute_script /home/ilutov/Developer/php-src/main/main.c:2482
    #16 0x1d856ab in do_cli /home/ilutov/Developer/php-src/sapi/cli/php_cli.c:964
    #17 0x1d87d13 in main %s:%d
    #18 0x7f73b0a4a50f in __libc_start_call_main (/lib64/libc.so.6+0x2750f)
    #19 0x7f73b0a4a5c8 in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x275c8)
    #20 0x603e24 in _start (/home/ilutov/Developer/php-src/sapi/cli/php+0x603e24)

0x60c000008998 is located 24 bytes inside of 128-byte region [0x60c000008980,0x60c000008a00)
freed by thread T0 here:
    #0 0x7f73b16b9388 in __interceptor_free.part.0 (/lib64/libasan.so.8+0xb9388)
    #1 0x1527f17 in tracked_free /home/ilutov/Developer/php-src/Zend/zend_alloc.c:2845
    #2 0x1526179 in _efree_custom /home/ilutov/Developer/php-src/Zend/zend_alloc.c:2480
    #3 0x1526486 in _efree /home/ilutov/Developer/php-src/Zend/zend_alloc.c:2600
    #4 0x1a9e9a9 in zend_ast_ref_destroy /home/ilutov/Developer/php-src/Zend/zend_ast.c:1176
    #5 0x162993b in rc_dtor_func /home/ilutov/Developer/php-src/Zend/zend_variables.c:57
    #6 0x15bc122 in zval_ptr_dtor_nogc /home/ilutov/Developer/php-src/Zend/zend_variables.h:35
    #7 0x15c8f8e in zval_update_constant_with_ctx /home/ilutov/Developer/php-src/Zend/zend_execute_API.c:699
    #8 0x15c92ba in zval_update_constant_ex /home/ilutov/Developer/php-src/Zend/zend_execute_API.c:710
    #9 0x17a29c6 in ZEND_FETCH_CLASS_CONSTANT_SPEC_CONST_CONST_HANDLER /home/ilutov/Developer/php-src/Zend/zend_vm_execute.h:7243
    #10 0x19de180 in execute_ex /home/ilutov/Developer/php-src/Zend/zend_vm_execute.h:57172
    #11 0x15cd115 in zend_call_function /home/ilutov/Developer/php-src/Zend/zend_execute_API.c:938
    #12 0x15cea26 in zend_call_known_function /home/ilutov/Developer/php-src/Zend/zend_execute_API.c:1032
    #13 0xf557b2 in spl_perform_autoload /home/ilutov/Developer/php-src/ext/spl/php_spl.c:448
    #14 0x15d0943 in zend_lookup_class_ex /home/ilutov/Developer/php-src/Zend/zend_execute_API.c:1198
    #15 0x15d2def in zend_fetch_class /home/ilutov/Developer/php-src/Zend/zend_execute_API.c:1620
    #16 0x15b83b1 in zend_get_class_constant_ex /home/ilutov/Developer/php-src/Zend/zend_constants.c:336
    #17 0x1a9a02a in zend_ast_evaluate_inner /home/ilutov/Developer/php-src/Zend/zend_ast.c:855
    #18 0x1a95d63 in zend_ast_evaluate_ex /home/ilutov/Developer/php-src/Zend/zend_ast.c:522
    #19 0x1a9614f in zend_ast_evaluate_inner /home/ilutov/Developer/php-src/Zend/zend_ast.c:544
    #20 0x1a95d63 in zend_ast_evaluate_ex /home/ilutov/Developer/php-src/Zend/zend_ast.c:522
    #21 0x15c8f62 in zval_update_constant_with_ctx /home/ilutov/Developer/php-src/Zend/zend_execute_API.c:696
    #22 0x15c92ba in zval_update_constant_ex /home/ilutov/Developer/php-src/Zend/zend_execute_API.c:710
    #23 0x17a29c6 in ZEND_FETCH_CLASS_CONSTANT_SPEC_CONST_CONST_HANDLER /home/ilutov/Developer/php-src/Zend/zend_vm_execute.h:7243
    #24 0x19de180 in execute_ex /home/ilutov/Developer/php-src/Zend/zend_vm_execute.h:57172
    #25 0x15cd115 in zend_call_function /home/ilutov/Developer/php-src/Zend/zend_execute_API.c:938
    #26 0x15cea26 in zend_call_known_function /home/ilutov/Developer/php-src/Zend/zend_execute_API.c:1032
    #27 0xf557b2 in spl_perform_autoload /home/ilutov/Developer/php-src/ext/spl/php_spl.c:448
    #28 0x15d0943 in zend_lookup_class_ex /home/ilutov/Developer/php-src/Zend/zend_execute_API.c:1198
    #29 0x15d3041 in zend_fetch_class_by_name /home/ilutov/Developer/php-src/Zend/zend_execute_API.c:1664

previously allocated by thread T0 here:
    #0 0x7f73b16ba6af in __interceptor_malloc (/lib64/libasan.so.8+0xba6af)
    #1 0x1527c50 in tracked_malloc /home/ilutov/Developer/php-src/Zend/zend_alloc.c:2826
    #2 0x1525fef in _malloc_custom /home/ilutov/Developer/php-src/Zend/zend_alloc.c:2471
    #3 0x15263bb in _emalloc /home/ilutov/Developer/php-src/Zend/zend_alloc.c:2590
    #4 0x1a9deb3 in zend_ast_copy /home/ilutov/Developer/php-src/Zend/zend_ast.c:1118
    #5 0x15ad394 in zend_const_expr_to_zval /home/ilutov/Developer/php-src/Zend/zend_compile.c:10147
    #6 0x158baf6 in zend_compile_class_const_decl /home/ilutov/Developer/php-src/Zend/zend_compile.c:7718
    #7 0x158bd8d in zend_compile_class_const_group /home/ilutov/Developer/php-src/Zend/zend_compile.c:7733
    #8 0x15ae289 in zend_compile_stmt /home/ilutov/Developer/php-src/Zend/zend_compile.c:10264
    #9 0x157864f in zend_compile_stmt_list /home/ilutov/Developer/php-src/Zend/zend_compile.c:6283
    #10 0x15ae0ef in zend_compile_stmt /home/ilutov/Developer/php-src/Zend/zend_compile.c:10202
    #11 0x158fc15 in zend_compile_class_decl /home/ilutov/Developer/php-src/Zend/zend_compile.c:7999
    #12 0x15adcb1 in zend_compile_top_stmt /home/ilutov/Developer/php-src/Zend/zend_compile.c:10177
    #13 0x15ad996 in zend_compile_top_stmt /home/ilutov/Developer/php-src/Zend/zend_compile.c:10166
    #14 0x14b0859 in zend_compile Zend/zend_language_scanner.l:619
    #15 0x14b277c in compile_string Zend/zend_language_scanner.l:815
    #16 0x173b0fc in zend_include_or_eval /home/ilutov/Developer/php-src/Zend/zend_execute.c:4878
    #17 0x1801a35 in ZEND_INCLUDE_OR_EVAL_SPEC_TMPVAR_HANDLER /home/ilutov/Developer/php-src/Zend/zend_vm_execute.h:14730
    #18 0x19e3820 in execute_ex /home/ilutov/Developer/php-src/Zend/zend_vm_execute.h:58068
    #19 0x15cd115 in zend_call_function /home/ilutov/Developer/php-src/Zend/zend_execute_API.c:938
    #20 0x15cea26 in zend_call_known_function /home/ilutov/Developer/php-src/Zend/zend_execute_API.c:1032
    #21 0xf557b2 in spl_perform_autoload /home/ilutov/Developer/php-src/ext/spl/php_spl.c:448
    #22 0x15d0943 in zend_lookup_class_ex /home/ilutov/Developer/php-src/Zend/zend_execute_API.c:1198
    #23 0x15d3041 in zend_fetch_class_by_name /home/ilutov/Developer/php-src/Zend/zend_execute_API.c:1664
    #24 0x17c99dd in ZEND_NEW_SPEC_CONST_UNUSED_HANDLER /home/ilutov/Developer/php-src/Zend/zend_vm_execute.h:10454
    #25 0x19dfe1e in execute_ex /home/ilutov/Developer/php-src/Zend/zend_vm_execute.h:57468
    #26 0x19f4e41 in zend_execute /home/ilutov/Developer/php-src/Zend/zend_vm_execute.h:60935
    #27 0x163aa29 in zend_execute_scripts /home/ilutov/Developer/php-src/Zend/zend.c:1798
    #28 0x13cfd7e in php_execute_script /home/ilutov/Developer/php-src/main/main.c:2482
    #29 0x1d856ab in do_cli /home/ilutov/Developer/php-src/sapi/cli/php_cli.c:964

SUMMARY: AddressSanitizer: heap-use-after-free /home/ilutov/Developer/php-src/Zend/zend_ast.c:546 in zend_ast_evaluate_inner
Shadow bytes around the buggy address:
  0x0c187fff90e0: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
  0x0c187fff90f0: fd fd fd fd fd fd fd fd fa fa fa fa fa fa fa fa
  0x0c187fff9100: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c187fff9110: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
  0x0c187fff9120: fd fd fd fd fd fd fd fd fa fa fa fa fa fa fa fa
=>0x0c187fff9130: fd fd fd[fd]fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c187fff9140: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
  0x0c187fff9150: 00 00 00 00 00 00 00 fa fa fa fa fa fa fa fa fa
  0x0c187fff9160: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c187fff9170: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c187fff9180: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==587089==ABORTING

The freed by thread T0 here: section is the most telling. Basically, we're entering zval_update_constant_ex twice for the same constant, once through zend_update_class_constants and once through ZEND_FETCH_CLASS_CONSTANT. The former will lock / check for a lock on the class constant zv but the latter will not. Thus, if you trigger ZEND_FETCH_CLASS_CONSTANT in spl_autoload_register on the same constant that's being evaluated the constant value will stlil be the constant AST, and zend_update_class_constants will be called again. This could happen in other ways (all the things that are adjusted in this PR), e.g. constant('B::HW'), via reflection, etc. When the nested zval_update_constant_ex call ends it will assign the evaluated result to the constant and free the constant AST. The outer zval_update_constant_ex call will later try to access the constant AST again which is now freed.

To solve this, I extracted the locking/checking mechanism into a separate function that is used specifically for class constants.

Do you see a simpler solution for this? At first, I thought I might be able to move it into zval_update_constant_ex to guard all constant ASTs. However, this function is used for more than just class constants (constants, default params, default properties, static variables, etc) and they might use zval.u2 for different purposes.

Unfortunately, while testing right now I also discovered that this can happen for evaluating default properties.

class B {
    public $prop = A::HW;
}

spl_autoload_register(function ($class) {
    class A { const HW = "this is A"; }
    var_dump(new B());
});

try {
    var_dump(new B());
} catch (Error $e) {
    echo $e->getMessage(), "\n";
}

new B() will evaluate default properties, which triggers the autoloader for A, which again instanciates B again triggering the evaluation of default properties. I'll need to look at all types of constant ASTs and whether something like this is possible for each case.

Hopefully this explanation was more useful than the issue description.

@iluuu1994 iluuu1994 mentioned this pull request Feb 27, 2023
Closed
@iluuu1994
Copy link
Member Author

Supersedes by #10718.

@iluuu1994 iluuu1994 closed this Feb 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dstogov dstogov Awaiting requested review from dstogov

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@iluuu1994 @dstogov