Skip to content

Commit

Permalink
Fix GH-9068: Conditional jump or move depends on uninitialised value(s)
Browse files Browse the repository at this point in the history
This patch preserves the scratch registers of the SysV x86-64 ABI by storing
them to the stack and restoring them later. We need to do this to prevent the
registers of the caller from being corrupted. The reason these get corrupted
is because the compiler is unaware of the Valgrind replacement function and
thus makes assumptions about the original function regarding registers which
are not true for the replacement function.

For implementation I used a GCC and Clang attribute. A more general
approach would be to use inline assembly but that's also less portable
and quite hacky. This attributes is supported since GCC 7.x, but the
target option is only supported since 11.x. For Clang the target option
does not matter.

Closes GH-10221.
  • Loading branch information
nielsdos committed May 3, 2023
1 parent 11597d1 commit 4ca8daf
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 1 deletion.
4 changes: 4 additions & 0 deletions NEWS
Expand Up @@ -2,6 +2,10 @@ PHP NEWS
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
?? ??? ????, PHP 8.1.20

- Core:
. Fixed bug GH-9068 (Conditional jump or move depends on uninitialised
value(s)). (nielsdos)

- Opcache:
. Fixed bug GH-11134 (Incorrect match default branch optimization). (ilutov)
. Fixed too wide OR and AND range inference. (nielsdos)
Expand Down
19 changes: 18 additions & 1 deletion Zend/zend_string.c
Expand Up @@ -372,11 +372,28 @@ ZEND_API void zend_interned_strings_switch_storage(bool request)
# define I_REPLACE_SONAME_FNNAME_ZU(soname, fnname) _vgr00000ZU_ ## soname ## _ ## fnname
#endif

ZEND_API bool ZEND_FASTCALL I_REPLACE_SONAME_FNNAME_ZU(NONE,zend_string_equal_val)(zend_string *s1, zend_string *s2)
/* See GH-9068 */
#if defined(__GNUC__) && (__GNUC__ >= 11 || defined(__clang__)) && __has_attribute(no_caller_saved_registers)
# define NO_CALLER_SAVED_REGISTERS __attribute__((no_caller_saved_registers))
# ifndef __clang__
# pragma GCC push_options
# pragma GCC target ("general-regs-only")
# define POP_OPTIONS
# endif
#else
# define NO_CALLER_SAVED_REGISTERS
#endif

ZEND_API bool ZEND_FASTCALL NO_CALLER_SAVED_REGISTERS I_REPLACE_SONAME_FNNAME_ZU(NONE,zend_string_equal_val)(zend_string *s1, zend_string *s2)
{
return !memcmp(ZSTR_VAL(s1), ZSTR_VAL(s2), ZSTR_LEN(s1));
}

#ifdef POP_OPTIONS
# pragma GCC pop_options
# undef POP_OPTIONS
#endif

#if defined(__GNUC__) && defined(__i386__)
ZEND_API bool ZEND_FASTCALL zend_string_equal_val(zend_string *s1, zend_string *s2)
{
Expand Down

1 comment on commit 4ca8daf

@nielsdos
Copy link
Member Author

Choose a reason for hiding this comment

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

This commit will degrade the performance on Valgrind runs only, but not on production or development builds that don't involve Valgrind.
This is because with this patch all the caller saved registers are pushed on the stack before calling memcmp, and after the call popped again.
This message is a heads up for the people running benchmarks. cc @iluuu1994 @dstogov

Please sign in to comment.