Skip to content

Commit

Permalink
Fix inline zend_string using struct padding
Browse files Browse the repository at this point in the history
As explained by Snape3058: On 64-bit machines, we typically have 7 bytes
of padding between the zend_string.val[0] char and the following char[].
This means that zend_string.val[1-7] write to and read from the struct
padding, which is a bad idea.

Allocate the given string separately instead.

Fixes GH-17564
Closes GH-17576
  • Loading branch information
iluuu1994 committed Jan 27, 2025
1 parent 556def7 commit 8ea9b04
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 16 deletions.
3 changes: 2 additions & 1 deletion NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ PHP NEWS
. Fix may_have_extra_named_args flag for ZEND_AST_UNPACK. (nielsdos)
. Fix NULL arithmetic in System V shared memory emulation for Windows. (cmb)


- DOM:
. Fixed bug GH-17500 (Segfault with requesting nodeName on nameless doctype).
(nielsdos)
Expand All @@ -43,6 +42,8 @@ PHP NEWS

- Opcache:
. Fixed bug GH-17307 (Internal closure causes JIT failure). (nielsdos)
. Fixed bug GH-17564 (Potential UB when reading from / writing to struct
padding). (ilutov)

- PDO:
. Fixed a memory leak when the GC is used to free a PDOStatment. (Girgias)
Expand Down
35 changes: 22 additions & 13 deletions ext/opcache/ZendAccelerator.c
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,8 @@ static void preload_restart(void);
# define LOCKVAL(v) (ZCSG(v))
#endif

#define ZCG_KEY_LEN (MAXPATHLEN * 8)

/**
* Clear AVX/SSE2-aligned memory.
*/
Expand Down Expand Up @@ -1190,7 +1192,8 @@ zend_string *accel_make_persistent_key(zend_string *str)
char *key;
int key_length;

ZSTR_LEN(&ZCG(key)) = 0;
ZEND_ASSERT(GC_REFCOUNT(ZCG(key)) == 1);
ZSTR_LEN(ZCG(key)) = 0;

/* CWD and include_path don't matter for absolute file names and streams */
if (IS_ABSOLUTE_PATH(path, path_length)) {
Expand Down Expand Up @@ -1300,7 +1303,7 @@ zend_string *accel_make_persistent_key(zend_string *str)
}

/* Calculate key length */
if (UNEXPECTED((size_t)(cwd_len + path_length + include_path_len + 2) >= sizeof(ZCG(_key)))) {
if (UNEXPECTED((size_t)(cwd_len + path_length + include_path_len + 2) >= ZCG_KEY_LEN)) {
return NULL;
}

Expand All @@ -1309,7 +1312,7 @@ zend_string *accel_make_persistent_key(zend_string *str)
* since in itself, it may include colons (which we use to separate
* different components of the key)
*/
key = ZSTR_VAL(&ZCG(key));
key = ZSTR_VAL(ZCG(key));
memcpy(key, path, path_length);
key[path_length] = ':';
key_length = path_length + 1;
Expand All @@ -1333,7 +1336,7 @@ zend_string *accel_make_persistent_key(zend_string *str)
parent_script_len = ZSTR_LEN(parent_script);
while ((--parent_script_len > 0) && !IS_SLASH(ZSTR_VAL(parent_script)[parent_script_len]));

if (UNEXPECTED((size_t)(key_length + parent_script_len + 1) >= sizeof(ZCG(_key)))) {
if (UNEXPECTED((size_t)(key_length + parent_script_len + 1) >= ZCG_KEY_LEN)) {
return NULL;
}
key[key_length] = ':';
Expand All @@ -1342,11 +1345,9 @@ zend_string *accel_make_persistent_key(zend_string *str)
key_length += parent_script_len;
}
key[key_length] = '\0';
GC_SET_REFCOUNT(&ZCG(key), 1);
GC_TYPE_INFO(&ZCG(key)) = GC_STRING;
ZSTR_H(&ZCG(key)) = 0;
ZSTR_LEN(&ZCG(key)) = key_length;
return &ZCG(key);
ZSTR_H(ZCG(key)) = 0;
ZSTR_LEN(ZCG(key)) = key_length;
return ZCG(key);
}

/* not use_cwd */
Expand Down Expand Up @@ -2014,8 +2015,8 @@ zend_op_array *persistent_compile_file(zend_file_handle *file_handle, int type)
ZCG(cache_opline) == EG(current_execute_data)->opline))) {

persistent_script = ZCG(cache_persistent_script);
if (ZSTR_LEN(&ZCG(key))) {
key = &ZCG(key);
if (ZSTR_LEN(ZCG(key))) {
key = ZCG(key);
}

} else {
Expand Down Expand Up @@ -2555,7 +2556,7 @@ static zend_string* persistent_zend_resolve_path(zend_string *filename)
SHM_PROTECT();
HANDLE_UNBLOCK_INTERRUPTIONS();
} else {
ZSTR_LEN(&ZCG(key)) = 0;
ZSTR_LEN(ZCG(key)) = 0;
}
ZCG(cache_opline) = EG(current_execute_data) ? EG(current_execute_data)->opline : NULL;
ZCG(cache_persistent_script) = persistent_script;
Expand Down Expand Up @@ -2927,7 +2928,15 @@ static void accel_globals_ctor(zend_accel_globals *accel_globals)
ZEND_TSRMLS_CACHE_UPDATE();
#endif
memset(accel_globals, 0, sizeof(zend_accel_globals));
accel_globals->key = zend_string_alloc(ZCG_KEY_LEN, true);
}

#ifdef ZTS
static void accel_globals_dtor(zend_accel_globals *accel_globals)
{
zend_string_free(accel_globals->key);
}
#endif

#ifdef HAVE_HUGE_CODE_PAGES
# ifndef _WIN32
Expand Down Expand Up @@ -3100,7 +3109,7 @@ static void accel_move_code_to_huge_pages(void)
static int accel_startup(zend_extension *extension)
{
#ifdef ZTS
accel_globals_id = ts_allocate_id(&accel_globals_id, sizeof(zend_accel_globals), (ts_allocate_ctor) accel_globals_ctor, NULL);
accel_globals_id = ts_allocate_id(&accel_globals_id, sizeof(zend_accel_globals), (ts_allocate_ctor) accel_globals_ctor, (ts_allocate_dtor) accel_globals_dtor);
#else
accel_globals_ctor(&accel_globals);
#endif
Expand Down
3 changes: 1 addition & 2 deletions ext/opcache/ZendAccelerator.h
Original file line number Diff line number Diff line change
Expand Up @@ -223,8 +223,7 @@ typedef struct _zend_accel_globals {
const zend_op *cache_opline;
zend_persistent_script *cache_persistent_script;
/* preallocated buffer for keys */
zend_string key;
char _key[MAXPATHLEN * 8];
zend_string *key;
} zend_accel_globals;

typedef struct _zend_string_table {
Expand Down

0 comments on commit 8ea9b04

Please sign in to comment.