Signed integer overflow in s_get_apply_fn

[mszabo-wikia]

While investigating an unrelated issue on a build of PHP + extensions compiled with sanitizers, I stumbled across the following UBSAN error, an apparent signed integer overflow:

/tmp/memcached/php_memcached.c:1442:3: runtime error: left shift of 65535 by 16 places cannot be represented in type 'int'
    #0 0x7f516eb5e44f in s_get_apply_fn /tmp/memcached/php_memcached.c:1442
    #1 0x7f516eb81e2c in php_memc_result_apply /tmp/memcached/php_memcached.c:691
    #2 0x7f516eb8441b in php_memc_mget_apply /tmp/memcached/php_memcached.c:759
    #3 0x7f516eb8ff2b in php_memc_get_impl /tmp/memcached/php_memcached.c:1492
    #4 0x5607e80b8a0e in ZEND_DO_FCALL_SPEC_RETVAL_USED_HANDLER /usr/src/php/Zend/zend_vm_execute.h:1863
    #5 0x5607e80b8a0e in execute_ex /usr/src/php/Zend/zend_vm_execute.h:55194
    #6 0x5607e80c2515 in zend_execute /usr/src/php/Zend/zend_vm_execute.h:59522
    #7 0x5607e7ac66d0 in zend_execute_scripts /usr/src/php/Zend/zend.c:1694
    #8 0x5607e77f75a3 in php_execute_script /usr/src/php/main/main.c:2545
    #9 0x5607e81fd943 in do_cli /usr/src/php/sapi/cli/php_cli.c:949
    #10 0x5607e65a656a in main /usr/src/php/sapi/cli/php_cli.c:1337
    #11 0x7f5178b30d09 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x23d09)
    #12 0x5607e65a7509 in _start (/usr/local/bin/php+0x2b6a509)

I would assume this does not actually break anything as existing code has likely come to rely on the undefined behavior.
I looked into what it would take to run tests with ASAN enabled, but so far it seems it may require a PHP binary itself compiled with ASAN, which is not ideal.

[sodabrew]

What version of the extension are you using? The line numbers are off by about 50 lines from current master

[mszabo-wikia]

Hey, thank you for the follow-up - this is with version 3.2.0 from PECL on PHP 8.0.25.

[mszabo-wikia]

https://github.com/php-memcached-dev/php-memcached/blob/v3.2.0/php_memcached.c#L1442

[remicollet]

@mszabo-wikia can you test if this minor change is enough ?

diff --git a/php_memcached.c b/php_memcached.c
index 7ccc9b5..ece5440 100644
--- a/php_memcached.c
+++ b/php_memcached.c
@@ -86,7 +86,7 @@ static int php_memc_list_entry(void) {
 /****************************************
   Payload value flags
 ****************************************/
-#define MEMC_CREATE_MASK(start, n_bits) (((1 << n_bits) - 1) << start)
+#define MEMC_CREATE_MASK(start, n_bits) (((1U << n_bits) - 1) << start)
 
 #define MEMC_MASK_TYPE     MEMC_CREATE_MASK(0, 4)
 #define MEMC_MASK_INTERNAL MEMC_CREATE_MASK(4, 12)

[remicollet]

ALso see pr #526 (you can try issue-522 branch)

[mszabo-wikia]

Thank you, I will try out the updated branch & let you know!

[mszabo-wikia]

The fixed branch seems to resolve the issue—I haven't yet had time to test to ensure it does not actually change the value of the flags, but this is promising!