Windows SHM reattachment fails when increasing memory_consumption or jit_buffer_size #18417

Activity

1. danog

   added

   Bug

   Status: Needs Triage

   on Apr 24, 2025
2. ndossche commented on Apr 24, 2025

   

   ndossche

   on Apr 24, 2025

   Last edited by ndossche

   Member

   More actions

   I can have a look. Were you running master or 8.4.6? The CI claims 8.4.6? Edit: oh you probably meant psalm master branch...
   I fixed a Windows JIT bug on monday which should make it in 8.4.7, so I wonder if it's related.
3. ndossche commented on Apr 24, 2025

   

   ndossche

   on Apr 24, 2025

   Member

   More actions

   I'm getting a crash in reattachment logic, investigating...
4. ndossche

   added

   Status: Verified

   and removed

   Status: Needs Triage

   on Apr 24, 2025
5. ndossche commented on Apr 24, 2025

   

   ndossche

   on Apr 24, 2025

   Last edited by ndossche

   Member

   More actions

   Explanation:

   1. Psalm first runs with a particular set shared memory size in requested_size.
   2. Psalm checks whether it should restart with more memory (https://github.com/vimeo/psalm/blob/f9fd6bc117e9ce1e854c2ed6777e7135aaa4966b/src/Psalm/Internal/Fork/PsalmRestarter.php#L79)
   3. A new psalm process launches with more memory, but tries to reattach to the same memory region. Note that this memory region was created with the old size (

      php-src/ext/opcache/shared_alloc_win32.c

      Lines 269 to 270 in c0da1bc

      	memfile = CreateFileMapping(INVALID_HANDLE_VALUE, NULL, PAGE_EXECUTE_READWRITE | SEC_COMMIT, size_high, size_low,
      	create_name_with_username(ACCEL_FILEMAP_NAME));

      )
   4. Reattaching fails here (

      php-src/ext/opcache/shared_alloc_win32.c

      Line 188 in c0da1bc

      if (!VirtualProtect(mapping_base, requested_size, PAGE_READWRITE, &old)) {

      ) because the requested_size is larger than the file mapping size created by the previous process. This fails.
   5. Crash

   It is not possible to resize file mappings as far as I know, so we'd have to make them unique per requested size. Although there is an undocumented NtExtendSection API, but I'm not so sure that relying on that is a good idea...

   A simple patch is including the requested size in the unique file mapping name. We could also try to include it in the zend_system_id but that may be overkill.

   This patch works for me:

   diff --git a/ext/opcache/shared_alloc_win32.c b/ext/opcache/shared_alloc_win32.c
   index 331aff32c98..b4f6ab5e83a 100644
   --- a/ext/opcache/shared_alloc_win32.c
   +++ b/ext/opcache/shared_alloc_win32.c
   @@ -69,9 +69,9 @@ static void zend_win_error_message(int type, char *msg, int err)
    	php_win32_error_msg_free(buf);
    }
    
   -static char *create_name_with_username(char *name)
   +static char *create_name_with_username(const char *name, size_t unique_id)
    {
   -	static char newname[MAXPATHLEN + 1 + 32 + 1 + 20 + 1 + 32 + 1];
   +	static char newname[MAXPATHLEN + 1 + 32 + 1 + 20 + 1 + 32 + sizeof("ffffffffffffffff")-1 + 1];
    	char *p = newname;
    	p += strlcpy(newname, name, MAXPATHLEN + 1);
    	*(p++) = '@';
   @@ -80,7 +80,9 @@ static char *create_name_with_username(char *name)
    	p += strlcpy(p, sapi_module.name, 21);
    	*(p++) = '@';
    	p = zend_mempcpy(p, zend_system_id, 32);
   -	*(p++) = '\0';
   +	if (unique_id) {
   +		p += snprintf(p, sizeof("ffffffffffffffff")-1, "%zx", unique_id);
   +	}
    	ZEND_ASSERT(p - newname <= sizeof(newname));
    
    	return newname;
   @@ -88,7 +90,7 @@ static char *create_name_with_username(char *name)
    
    void zend_shared_alloc_create_lock(void)
    {
   -	memory_mutex = CreateMutex(NULL, FALSE, create_name_with_username(ACCEL_MUTEX_NAME));
   +	memory_mutex = CreateMutex(NULL, FALSE, create_name_with_username(ACCEL_MUTEX_NAME, 0));
    	if (!memory_mutex) {
    		zend_accel_error(ACCEL_LOG_FATAL, "Cannot create mutex (error %u)", GetLastError());
    		return;
   @@ -222,7 +224,7 @@ static int create_segments(size_t requested_size, zend_shared_segment ***shared_
    	   can be called before the child process is killed. In this case, the mapping will fail
    	   and we have to sleep some time (until the child releases the mapping object) and retry.*/
    	do {
   -		memfile = OpenFileMapping(FILE_MAP_READ|FILE_MAP_WRITE|FILE_MAP_EXECUTE, 0, create_name_with_username(ACCEL_FILEMAP_NAME));
   +		memfile = OpenFileMapping(FILE_MAP_READ|FILE_MAP_WRITE|FILE_MAP_EXECUTE, 0, create_name_with_username(ACCEL_FILEMAP_NAME, requested_size));
    		if (memfile == NULL) {
    			err = GetLastError();
    			break;
   @@ -267,7 +269,7 @@ static int create_segments(size_t requested_size, zend_shared_segment ***shared_
    	(*shared_segments_p)[0] = shared_segment;
    
    	memfile	= CreateFileMapping(INVALID_HANDLE_VALUE, NULL, PAGE_EXECUTE_READWRITE | SEC_COMMIT, size_high, size_low,
   -								create_name_with_username(ACCEL_FILEMAP_NAME));
   +								create_name_with_username(ACCEL_FILEMAP_NAME, requested_size));
    	if (memfile == NULL) {
    		err = GetLastError();
    		zend_shared_alloc_unlock_win32();
   

   EDIT: just noticed now that I should terminate the string properly if unique_id is 0... Will do that in my PR.

   ❤️React with ❤️1Reacted by Daniil Gentili
6. ndossche

   added

   Extension: opcache

   on Apr 24, 2025
7. ndossche

   changed the title [-]Failing Psalm unit tests on windows with function JIT[/-] [+]Windows SHM reattachment fails when increasing memory_consumption or jit_buffer_size[/+] on Apr 27, 2025
8. ndossche

   added a commit that references this issue on Apr 27, 2025

   Fix phpGH-18417: Windows SHM reattachment fails when increasing memor…

   ...

   This issue will close once commit b911575 is merged into the 'master' branch.

   b911575
9. ndossche

   linked a pull request that will close this issueFix GH-18417: Windows SHM reattachment fails when increasing memory_consumption or jit_buffer_size #18443on Apr 27, 2025
10. khmehedi30 commented on Apr 28, 2025

    

    khmehedi30

    on Apr 28, 2025

    More actions

    nice work!
11. ndossche

    closed this as completedin 7869af6on Apr 28, 2025
12. ndossche

    added 3 commits that reference this issue on Apr 28, 2025

    Merge branch 'PHP-8.3' into PHP-8.4

    ...

    This issue will close once commit c620fee is merged into the 'master' branch.

    c620fee

    Merge branch 'PHP-8.4'

    ...

    This issue will close once commit f1b7335 is merged into the 'master' branch.

    f1b7335

    Fix GH-18417: Windows SHM reattachment fails when increasing memory_c…

    ...

    This issue will close once commit 7869af6 is merged into the 'master' branch.

    7869af6
13. danog commented on Apr 28, 2025

    

    danog

    on Apr 28, 2025

    ContributorAuthor

    More actions

    Thank you @nielsdos!