Skip to content
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.

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

Null pointer deref in observer API when calling cases() method on preloaded enum #17715

Open
TimWolla opened this issue Feb 6, 2025 · 4 comments

Comments

@TimWolla
Copy link
Member

TimWolla commented Feb 6, 2025

Description

The following code:

test5.php

<?php

spl_autoload_register(static function ($class) {
	if ($class === 'MyEnum') {
		require_once(__DIR__ . '/preload.php');
	}
});

var_dump(MyEnum::cases());

preload.php

<?php

enum MyEnum
{
	case Foo;
}

executed as:

sapi/cli/php -d zend_extension=$(pwd)/modules/opcache.so -d opcache.enable_cli=1 -d opcache.preload=$(pwd)/preload.php -d zend_test.observer.enabled=1 -d zend_test.observer.observe_all=1 test5.php 

Resulted in this output:

<!-- init 'php-src/preload.php' -->
<file 'php-src/preload.php'>
</file 'php-src/preload.php'>
<!-- init 'php-src/test5.php' -->
<file 'php-src/test5.php'>
  <!-- init spl_autoload_register() -->
  <spl_autoload_register>
  </spl_autoload_register>
Zend/zend_observer.h:108:82: runtime error: applying zero offset to null pointer
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior Zend/zend_observer.h:108:82 in 
Zend/zend_observer.h:92:9: runtime error: load of null pointer of type 'zend_observer_fcall_begin_handler' (aka 'void (*)(struct _zend_execute_data *)')
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior Zend/zend_observer.h:92:9 in 
AddressSanitizer:DEADLYSIGNAL
=================================================================
==511299==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x55c2312b95f7 bp 0x7fff01e61950 sp 0x7fff01e61940 T0)
==511299==The signal is caused by a READ memory access.
==511299==Hint: address points to the zero page.
    #0 0x55c2312b95f7 in zend_observer_handler_is_unobserved php-src/Zend/zend_observer.h:92:18
    #1 0x55c2312b9571 in zend_observer_fcall_has_no_observers php-src/Zend/zend_observer.h:109:9
    #2 0x55c2312b8d30 in zend_observer_fcall_begin_specialized php-src/Zend/zend_observer.h:115:7
    #3 0x55c230ecc1a7 in ZEND_DO_FCALL_SPEC_OBSERVER_HANDLER php-src/Zend/zend_vm_execute.h:2153:3
    #4 0x55c230c29d87 in execute_ex php-src/Zend/zend_vm_execute.h:58595:7
    #5 0x55c230c2afa2 in zend_execute php-src/Zend/zend_vm_execute.h:64247:2
    #6 0x55c2317528e8 in zend_execute_script php-src/Zend/zend.c:1943:3
    #7 0x55c2302e1ae6 in php_execute_script_ex php-src/main/main.c:2584:13
    #8 0x55c2302e2388 in php_execute_script php-src/main/main.c:2624:9
    #9 0x55c23176103a in do_cli php-src/sapi/cli/php_cli.c:946:5
    #10 0x55c23175ca32 in main php-src/sapi/cli/php_cli.c:1346:18
    #11 0x7f4cd2a2a1c9 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #12 0x7f4cd2a2a28a in __libc_start_main csu/../csu/libc-start.c:360:3
    #13 0x55c22e403374 in _start (php-src/sapi/cli/php+0x1c03374) (BuildId: 3ddbca74c8670516864203a517f20c02cff6881a)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV php-src/Zend/zend_observer.h:92:18 in zend_observer_handler_is_unobserved
==511299==ABORTING

But I expected this output instead:

<!-- init 'php-src/test5.php' -->
<file 'php-src/test5.php'>
  <!-- init spl_autoload_register() -->
  <spl_autoload_register>
  </spl_autoload_register>
  <!-- init MyEnum::cases() -->
  <MyEnum::cases>
  </MyEnum::cases>
  <!-- init var_dump() -->
  <var_dump>
array(1) {
  [0]=>
  enum(MyEnum::Foo)
}
  </var_dump>
</file 'php-src/test5.php'>

PHP Version

git master

Operating System

No response

@TimWolla TimWolla changed the title Null pointer deref in observer API when calling method on preloaded enum Null pointer deref in observer API when calling cases() method on preloaded enum Feb 6, 2025
@nielsdos
Copy link
Member

nielsdos commented Feb 6, 2025

Repros on 8.4 and master, seemingly not on 8.3.

@nielsdos
Copy link
Member

nielsdos commented Feb 6, 2025

Reverting 25d7616 fixes this

@nielsdos
Copy link
Member

nielsdos commented Feb 6, 2025

Yeah okay.
Preloading happens during post startup, where it copies the user classes to the CG class table, including the cases function. But setting up the persistent runtime cache happens prior to preloading. Moving it after the post startup will break the code that copies the runtime cache pointer.
cc @bwoebi

@bwoebi bwoebi self-assigned this Feb 14, 2025
bwoebi added a commit to bwoebi/php-src that referenced this issue Feb 16, 2025
This solely affects the builtin enum functions currently.

Given that these are stored in SHM, we cannot simply hardwire a pointer into the internal function runtime cache on NTS too, but have to use a MAP_PTR (like on ZTS).
Now, by design, the runtime cache of internal functions no longer is reset between requests, hence we need to store them explicitly as static runtime cache.

On NTS builds we cannot trivially move the pointers into CG(internal_run_time_cache) as they're directly stored on the individual functions (on ZTS we could simply iterate the static map_ptrs).
Hence, we have the choice between having opcache managing the internal run_time_cache for its preloaded functions itself or realloc CG(internal_run_time_cache) and iterate through all functions to assign the new address. We choose the latter for simplicity and initial speed.
bwoebi added a commit to bwoebi/php-src that referenced this issue Feb 16, 2025
This solely affects the builtin enum functions currently.

Given that these are stored in SHM, we cannot simply hardwire a pointer into the internal function runtime cache on NTS too, but have to use a MAP_PTR (like on ZTS).
Now, by design, the runtime cache of internal functions no longer is reset between requests, hence we need to store them explicitly as static runtime cache.

On NTS builds we cannot trivially move the pointers into CG(internal_run_time_cache) as they're directly stored on the individual functions (on ZTS we could simply iterate the static map_ptrs).
Hence, we have the choice between having opcache managing the internal run_time_cache for its preloaded functions itself or realloc CG(internal_run_time_cache) and iterate through all functions to assign the new address. We choose the latter for simplicity and initial speed.
bwoebi added a commit to bwoebi/php-src that referenced this issue Feb 16, 2025
This solely affects the builtin enum functions currently.

Given that these are stored in SHM, we cannot simply hardwire a pointer into the internal function runtime cache on NTS too, but have to use a MAP_PTR (like on ZTS).
Now, by design, the runtime cache of internal functions no longer is reset between requests, hence we need to store them explicitly as static runtime cache.

On NTS builds we cannot trivially move the pointers into CG(internal_run_time_cache) as they're directly stored on the individual functions (on ZTS we could simply iterate the static map_ptrs).
Hence, we have the choice between having opcache managing the internal run_time_cache for its preloaded functions itself or realloc CG(internal_run_time_cache) and iterate through all functions to assign the new address. We choose the latter for simplicity and initial speed.

Note: map_ptr_static_last has been added as last element to zend_accel_shared_globals, so that accesses to it are compatible. We do not have to care about the ABI of creating new zend_accel_shared_globals structs. That's opcaches prerogative.
@bwoebi
Copy link
Member

bwoebi commented Feb 16, 2025

@nielsdos Fixing this was non-trivial :-( I hope #17835 will do. If you're willing to check it out...

bwoebi added a commit to bwoebi/php-src that referenced this issue Feb 16, 2025
This solely affects the builtin enum functions currently.

Given that these are stored in SHM, we cannot simply hardwire a pointer into the internal function runtime cache on NTS too, but have to use a MAP_PTR (like on ZTS).
Now, by design, the runtime cache of internal functions no longer is reset between requests, hence we need to store them explicitly as static runtime cache.

On NTS builds we cannot trivially move the pointers into CG(internal_run_time_cache) as they're directly stored on the individual functions (on ZTS we could simply iterate the static map_ptrs).
Hence, we have the choice between having opcache managing the internal run_time_cache for its preloaded functions itself or realloc CG(internal_run_time_cache) and iterate through all functions to assign the new address. We choose the latter for simplicity and initial speed.

Note: map_ptr_static_last has been added as last element to zend_accel_shared_globals, so that accesses to it are compatible. We do not have to care about the ABI of creating new zend_accel_shared_globals structs. That's opcaches prerogative.
bwoebi added a commit to DataDog/dd-trace-php that referenced this issue Feb 17, 2025
…th observers

See also php/php-src#17715.

Relatively simple fix: just initialize the cache slots on every request. Has a slight per request-overhead, but avoids crashes when SomePreloadedEnum::cases().

Signed-off-by: Bob Weinand <[email protected]>
bwoebi added a commit to DataDog/dd-trace-php that referenced this issue Feb 17, 2025
…th observers

See also php/php-src#17715.

Relatively simple fix: just initialize the cache slots on every request. Has a slight per request-overhead, but avoids crashes when SomePreloadedEnum::cases().
bwoebi added a commit to DataDog/dd-trace-php that referenced this issue Feb 19, 2025
…th observers

See also php/php-src#17715.

Relatively simple fix: just initialize the cache slots on every request. Has a slight per request-overhead, but avoids crashes when SomePreloadedEnum::cases().

Signed-off-by: Bob Weinand <[email protected]>
bwoebi added a commit to DataDog/dd-trace-php that referenced this issue Feb 19, 2025
…th observers

See also php/php-src#17715.

Relatively simple fix: just initialize the cache slots on every request. Has a slight per request-overhead, but avoids crashes when SomePreloadedEnum::cases().

Signed-off-by: Bob Weinand <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants