Bug in refactoring in PHP 8.2 Windows shmat() function

[TysonAndre]

Description

The following code:

See krakjoe/apcu#445 for the original report

and #8648

• The shmat function will now return something other than -1 if the address wasn't allocated, due to checking an offset of a pointer against null instead of checking the original pointer. That seems likely to be the bug affecting windows users of APCu when certain web servers are used on Windows (I'm not familiar with windows APIs)

Resulted in this output:

(apcu (in-memory shared cache) suddenly sharing memory between different php processes in php 8.1.6

But I expected this output instead:

apcu behavior remains the same as php 8.1.5

PHP Version

8.2 and newer

Operating System

Windows

Suggested fix

(I don't have visual studio or Windows to test this with the bug report for apcu_fetch on PHP 8.1.6+ myself, so I don't know if it's already been fixed or can be reproduced by others yet)

--- a/TSRM/tsrm_win32.c
+++ b/TSRM/tsrm_win32.c
@@ -690,14 +690,14 @@ TSRM_API void *shmat(int key, const void *shmaddr, int flags)
                return (void*)-1;
        }
 
-       shm->addr = shm->descriptor + sizeof(shm->descriptor);
-
-       if (NULL == shm->addr) {
+       if (NULL == shm->descriptor) {
                int err = GetLastError();
                SET_ERRNO_FROM_WIN32_CODE(err);
                return (void*)-1;
        }
 
+       shm->addr = shm->descriptor + sizeof(shm->descriptor);

[cmb69]

Thanks for reporting! While you are right that this change was not correct, it was only applied to PHP-8.2, so is unlikely the cause of the APCu issue. Anyhow, I'll have a closer look at both.

[TysonAndre]

I'm not certain which version it started in, then, my original bug report was inaccurate.

• They'd upgraded from PHP 7 to PHP 8, so it may be a much older change.
• I'd only seen this issue yesterday, so I hadn't asked for clarification
• I don't know if it's part of their ini settings, fpm settings, ini settings, apcu ini settings, etc
• I still haven't confirmed if it's zts or nts