Windows SHM reattachment fails when increasing memory_consumption or jit_buffer_size

[danog]

Description

Description

Psalm end to end tests on the master branch are segfaulting with function JIT enabled.

https://github.com/vimeo/psalm/actions/runs/14579531189/job/41077468008

To reproduce, see vimeo/psalm#11402 and start the unit tests.

Ping @dstogov, @nielsdos.

PHP Version

PHP 8.4.6

Operating System

Microsoft Windows Server 2022 10.0.20348

[ndossche]

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.

[ndossche]

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

[ndossche]

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.

[khmehedi30]

nice work!