Skip to content

Detect misuse of ZEND_MAP_PTR_NEW() #16468

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Detect misuse of ZEND_MAP_PTR_NEW() #16468

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
00f6ac5
Detect misuse of ZEND_MAP_PTR_NEW()
arnaud-lb Oct 16, 2024
bfa3f9d
Review
arnaud-lb Oct 17, 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
17 changes: 17 additions & 0 deletions Zend/zend.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -737,6 +737,9 @@ static void compiler_globals_ctor(zend_compiler_globals *compiler_globals) /* {{
compiler_globals->map_ptr_base = ZEND_MAP_PTR_BIASED_BASE(NULL);
compiler_globals->map_ptr_size = 0;
compiler_globals->map_ptr_last = global_map_ptr_last;
#if ZEND_DEBUG
compiler_globals->map_ptr_locked = false;
#endif
compiler_globals->internal_run_time_cache = NULL;
if (compiler_globals->map_ptr_last || zend_map_ptr_static_size) {
/* Allocate map_ptr table */
Expand Down Expand Up @@ -788,6 +791,9 @@ static void compiler_globals_dtor(zend_compiler_globals *compiler_globals) /* {{
compiler_globals->map_ptr_real_base = NULL;
compiler_globals->map_ptr_base = ZEND_MAP_PTR_BIASED_BASE(NULL);
compiler_globals->map_ptr_size = 0;
#if ZEND_DEBUG
compiler_globals->map_ptr_locked = false;
#endif
}
if (compiler_globals->internal_run_time_cache) {
pefree(compiler_globals->internal_run_time_cache, 1);
Expand Down Expand Up @@ -1204,6 +1210,9 @@ void zend_shutdown(void) /* {{{ */
CG(map_ptr_real_base) = NULL;
CG(map_ptr_base) = ZEND_MAP_PTR_BIASED_BASE(NULL);
CG(map_ptr_size) = 0;
# if ZEND_DEBUG
CG(map_ptr_locked) = false;
# endif
}
if (CG(script_encoding_list)) {
free(ZEND_VOIDP(CG(script_encoding_list)));
Expand Down Expand Up @@ -2011,6 +2020,10 @@ ZEND_API void *zend_map_ptr_new(void)
{
void **ptr;

#if ZEND_DEBUG
ZEND_ASSERT((!startup_done || CG(in_compilation) || CG(map_ptr_locked)) && "Can not allocate map ptrs outside of startup and compilation");
Copy link
Member

@dstogov dstogov Oct 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The CG(in_compilation) looks suspicious. Is it necessary for PHP without opcache?

I think, it may be better to use CG(map_ptr_protected) initialized by false, then setting it to true in opcache post_starup and then updated by lock/unlock.

Copy link
Member Author

Choose a reason for hiding this comment

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

Without opcache ZEND_MAP_PTR_NEW() is always safe, but I think it's better to implement the same restrictions when opcache is not enabled, so that code that works without opcache also works with it.

The CG(in_compilation) looks suspicious

Indeed. I was convinced that compilation occurred while opcache was locked, so this was to handle the non-opcache case, but actually it's not.

I've simplified as you suggested, but I still deny usage of ZEND_MAP_PTR_NEW() after startup when opcache is not enabled.

Copy link
Member

Choose a reason for hiding this comment

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

Without opcache, map_ptr buffer belongs to process/thread and it may allocate new slots at any time.

With opcache. map_ptr buffer is addressed through opcache SHM, so we shouldn't add new slots without opcache lock. I think, protection should be obtained and managed by opcache. In case, we manage it outside opcache (e.g. in zend_alloc_ce_cache() we just negate the protection. (I may be wrong)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, zend_alloc_ce_cache() is what originally convinced me that the lock was held during compilation, since it calls ZEND_MAP_PTR_NEW_OFFSET(). But actually, the conditions in zend_alloc_ce_cache() ensure that we never call ZEND_MAP_PTR_NEW_OFFSET() after startup when opcache is enabled (a map ptr is only allocated when the class name is interned and not permanent, which only happens without opcache).

Copy link
Member

Choose a reason for hiding this comment

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

I still think that the modification of CG(map_ptr_protected) in zend_alloc_ce_cache() is a bad decision.
The protection should have a single point of responsibility.

But actually, the conditions in zend_alloc_ce_cache() ensure that we never call ZEND_MAP_PTR_NEW_OFFSET() after startup when opcache is enabled (a map ptr is only allocated when the class name is interned and not permanent, which only happens without opcache).

This is too complex verbal condition and it may be easily violated by inaccurate future modification.
You introduce protection exactly against inaccurate future modifications, so it's better not to make exceptions without a big reason.

#endif

if (CG(map_ptr_last) >= CG(map_ptr_size)) {
/* Grow map_ptr table */
CG(map_ptr_size) = ZEND_MM_ALIGNED_SIZE_EX(CG(map_ptr_last) + 1, 4096);
Expand All @@ -2027,6 +2040,10 @@ ZEND_API void *zend_map_ptr_new_static(void)
{
void **ptr;

#if ZEND_DEBUG
ZEND_ASSERT(!startup_done && "Can not allocate static map ptrs after startup");
#endif

if (zend_map_ptr_static_last >= zend_map_ptr_static_size) {
zend_map_ptr_static_size += 4096;
/* Grow map_ptr table */
Expand Down
3 changes: 3 additions & 0 deletions Zend/zend_globals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,9 @@ struct _zend_compiler_globals {
void *map_ptr_base;
size_t map_ptr_size;
size_t map_ptr_last;
#if ZEND_DEBUG
bool map_ptr_locked;
#endif

HashTable *delayed_variance_obligations;
HashTable *delayed_autoloads;
Expand Down
6 changes: 6 additions & 0 deletions ext/opcache/zend_shared_alloc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -515,6 +515,9 @@ void zend_shared_alloc_lock(void)
#endif

ZCG(locked) = 1;
#if ZEND_DEBUG
CG(map_ptr_locked) = true;
#endif
}

void zend_shared_alloc_unlock(void)
Expand All @@ -530,6 +533,9 @@ void zend_shared_alloc_unlock(void)
mem_write_unlock.l_len = 1;
#endif

#if ZEND_DEBUG
CG(map_ptr_locked) = false;
#endif
ZCG(locked) = 0;

#ifndef ZEND_WIN32
Expand Down