Fix GH-14643 ext/standard: segfault on user shutdown function release. #14656
Conversation
ext/standard/basic_functions.c
Outdated
| zend_fcall_info *fci = &shutdown_function_entry->fci; | ||
|
|
||
| zend_fcall_info_args_clear(&shutdown_function_entry->fci, true); | ||
| fci_release(&shutdown_function_entry->fci, &shutdown_function_entry->fci_cache); | ||
| if (fci) { | ||
| zend_fcall_info_args_clear(fci, true); | ||
| fci_release(fci, &shutdown_function_entry->fci_cache); | ||
| } |
There was a problem hiding this comment.
This seems to point to a bug elsewhere it should not be possible to not have an FCI here.
There was a problem hiding this comment.
Furthermore, the patch also doesn't really make sense: fci is address-taken from a struct member of shutdown_function_entry, so it can only be NULL if shutdown_function_entry is NULL, but in that case it's undefined behaviour because you aren't allowed to do pointer math on NULL...
I think we should all run our development tools with ASAN + UBSAN enabled.
There was a problem hiding this comment.
Agreed that we should check shutdown_function_entry here, and not shutdown_function_entry->fci.
My understanding of the issue is that we OOM here when registering a new shutdown function:
Line 858 in 909d331
as a consequence,
Z_PTR_P(zv) remains NULL (it is initialized to NULL above), so shutdown_function_entry is NULL here: php-src/ext/standard/basic_functions.c
Line 1584 in 909d331
So the root cause is zend_hash_next_index_insert_mem(). We should probably change this function so that it doesn't leave NULL entries in case of allocation failure. We could move the malloc/memcpy before the insert, initialize tmp to that, and free when insert fails. WDYT?
There was a problem hiding this comment.
I think moving the allocation before the insert is the right approach.
There was a problem hiding this comment.
Thanks all for your feedback, just trying to take care of "day to day" stuff as much as I can :) but at least I get better understanding of the internals.
Zend/zend_hash.h
Outdated
| memcpy(Z_PTR_P(zv), pData, size); | ||
| return Z_PTR_P(zv); | ||
| void *p = pemalloc(size, GC_FLAGS(ht) & IS_ARRAY_PERSISTENT); | ||
| if (!p) { |
There was a problem hiding this comment.
pemalloc can't fail because either it's using the request allocator which can't fail, or it's using the persistent allocator __zend_malloc which has a check:
Lines 3320 to 3323 in 4f450b6
Zend/zend_hash.h
Outdated
|
|
||
| memcpy(p, pData, size); | ||
| ZVAL_PTR(&tmp, p); | ||
| if (!(zv = zend_hash_next_index_insert(ht, &tmp))) { |
There was a problem hiding this comment.
I wouldn't assign the return value but I'd simply return p at the end.
No description provided.