Fix zend_alloc aligned allocation on Windows

[mvorisek]

originally, on Windows, the memory was aligned by an assumption the original memory is available for immediately following allocation

even if the aligned allocation was inside the original address space, it seems this assumption is not always correct and the aligned allocation might fail, especially in 32-builds where is address space is much smaller

fixes #9650 and https://bugs.php.net/bug.php?id=77194

also no need to dump errno on linux twice, it is dumped already when reporting lower level alloc failures (mmap, nmmap, munmap)

[cmb69]

Please target the "master" branch.

[TysonAndre]

So if VirtualAlloc fails on win32 when realigning the pointer, was your plan to allocate a larger block of size size + alignment that wasn't fixed, then align it (e.g. by unmapping extra memory from the start and end where needed)? (That's what I would try)

EDIT: I assume it's doing that but still failing - I assume the call to zend_mm_mmap_fixed is entirely within the region that was just unmapped

		ptr = zend_mm_mmap(size + alignment - REAL_PAGE_SIZE);
#ifdef _WIN32
		offset = ZEND_MM_ALIGNED_OFFSET(ptr, alignment);
		zend_mm_munmap(ptr, size + alignment - REAL_PAGE_SIZE);

#define ZEND_MM_CHUNK_SIZE ((size_t) (2 * 1024 * 1024))    /* 2 MB  */

https://learn.microsoft.com/en-us/cpp/preprocessor/predefined-macros?view=msvc-170
_WIN32 Defined as 1 when the compilation target is 32-bit ARM, 64-bit ARM, x86, or x64. Otherwise, undefined.

This is a bug fix for a random crash due to getting unlucky allocating a memory page that affects 32-bit windows, so my preference would be targeting 8.0 myself.

Maybe

• Only change the handling of the fixed mapping returning null when #ifdef _WIN32
• Change the logging in php's master branch for 8.3.0-dev

[TysonAndre]

https://stackoverflow.com/a/14816824

Are we sure that the page size is actually 4KB at runtime in appveyor? Maybe VirtualAlloc is somehow using a large page size for VirtualAlloc, and as a result it's rounding up to the next 8KB, and as a result it's more likely to request mmap_fixed after the end of a region

https://learn.microsoft.com/en-us/windows/win32/api/sysinfoapi/nf-sysinfoapi-getsysteminfo can be called to check the page size at runtime

[in] dwSize

The size of the region, in bytes. If the lpAddress parameter is NULL, this value is rounded up to the next page boundary. Otherwise, the allocated pages include all pages containing one or more bytes in the range from lpAddress to lpAddress+dwSize. This means that a 2-byte range straddling a page boundary causes both pages to be included in the allocated region.

#define ZEND_MM_PAGE_SIZE  (4 * 1024)                      /* 4 KB  */

[cmb69]

Maybe

• Only change the handling of the fixed mapping returning null when #ifdef _WIN32
• Change the logging in php's master branch for 8.3.0-dev

Right. If there is a real bug, it should be fixed in PHP-8.0. But a sloppy error reporting doesn't look like a real bug to me.

[TysonAndre]

More brainstorming:

https://github.com/microsoft/mimalloc/blob/master/src/os.c#L411-L422 https://learn.microsoft.com/en-us/windows/win32/api/memoryapi/nf-memoryapi-virtualalloc2 - it seems like VirtualAlloc2 supports huge pages as well as passing the requested alignment to the VirtualAlloc2 call itself, but only works in windows 10+ (that seems too new, but it's useful to know about)

Zend/zend.c already defines a helper to get the page size - ZEND_API size_t zend_get_page_size() - we could print out the value on appveyor for the x86 platform (_WIN32) (e.g. maybe add it to phpinfo or zend_test so we can assert that 32-bit builds have the expected page size of 4kb on windows and not something larger)?

[mvorisek]

I did some more debugging on x86 Win build:

# random test failure I:
XXXX_MM_LOG_XXXX:
alloc: null, 0CCB0000, 200000
free: 0CCB0000, 200000
alloc: null, 0CCB0000, 3ff000
free: 0CCB0000, 3ff000
alloc: 0CE00000, 00000000, 200000

XXXX_MM_LOG_XXXX:
free: 0CE00000, 200000
    VirtualFree() failed: [0x000001e7] Attempt to access invalid address
    Can't initialize heap


# random test failure II:
XXXX_MM_LOG_XXXX:
alloc: null, 0C300000, 200000
free: 0C300000, 200000
alloc: null, 0C300000, 3ff000
free: 0C300000, 3ff000
alloc: 0C400000, 00000000, 200000

XXXX_MM_LOG_XXXX:
free: 0C400000, 200000
    VirtualFree() failed: [0x000001e7] Attempt to access invalid address
    Can't initialize heap

# ext\gd\tests\bug77269.phpt (test needs to be fixed fo x86, unrelated):
alloc: null, 0BF00000, 200000
free: 0BF00000, 200000
alloc: null, 0BF00000, 3ff000
free: 0BF00000, 3ff000
alloc: 0C000000, 0C000000, 200000
alloc: null, 29000000, 10000000
alloc: null, 00000000, 40000000
    VirtualAlloc(NULL) failed: [0x00000008] Not enough memory resources are available to process this command

syntax is:

alloc: orig_ptr, new_ptr, new_size === new_ptr = VirtualAlloc(orig_ptr, new_size)
free: ptr, size === VirtualFree(ptr, size)

PHP zend_alloc is extending the memory by passing the next address and tries to allocate it. This have at least two problems:

1. invalid address can be passed if the previous virtual address was on the very end of the valid space

like passing value greater than 0x7FFFFFFF in x86 Win build

2. if allocation fails, php currently tries to free the memory (ptr passed to the alloc func), this is wrong, as passed pointer might be not previously allocated (as shown in the log, in the code, we must pass an extra arg to zend_mm_mmap_fixed if ptr is wanted or already allocated)

[TysonAndre]

php > echo dechex(0xC300000+0x3ff000);
c6ff000
php > echo dechex(0xC400000+0x200000);
c600000

From the trace you posted, it looks like the free then alloc is

1. allocating 0xC300000-0xC6ff000 with VirtualAlloc
2. immediately freeing it with VirtualFree, then
3. allocating 0xC400000-0xC600000, which is within the range of the memory that was just freed before. (block start and end chosen to be aligned to 2MB, i.e. 0x200000),

So the surprise seems to be that calling VirtualFree then immediately calling VirtualAlloc on a region entirely within the memory freed by VirtualFree doesn't always work.

Brainstorming:

• Maybe the OS is doing something that will use memory in that page immediately after the call to VirtualAlloc
• Maybe another thread (e.g. for input, signal handling, process communication) picked up the page immediately after VirtualFree freed it before VirtualAlloc could be called

https://learn.microsoft.com/en-us/windows-hardware/drivers/gettingstarted/virtual-address-spaces - each process's virtual memory space is distinct, so it probably isn't the action of other processes

---

Calling VirtualFree with null seems like it would have no user impact, but could be fixed regardless

It'd be useful to have to get the actual error code from VirtualAlloc instead of the 0x000001e7 from VirtualFree for VirtualFree() failed: [0x000001e7] Attempt to access invalid address

[TysonAndre]

My best idea for a workaround is to retry the entire request for aligned memory (alloc with extra + free + alloc aligned at pointer) on win32 if it fails, a finite number of times (in case there's actually no memory or another error)

• Probably also including the happy path of requesting ZEND_MM_CHUNK_SIZE=2MB and returning early if it's already aligned to ZEND_MM_CHUNK_SIZE
• If you can't allocate any memory whatsoever with the request at address NULL(requesting any address), then exit early

E.g. 10 times. If this bug (race condition?) is only encountered 1 in 10000 times independently (not actually independent), then 1 in 10000¹⁰ chance of it happening would be a better user experience

If that fails, then return a fatal error like it did before for being unable to allocate memory

---

For Windows 10, https://github.com/microsoft/mimalloc/blob/master/src/os.c# has an example of calling GetProcAddress to look up the function pointer of VirtualAlloc2, which lets you pass in the alignment hint to the allocator

• That also gets freed by VirtualFree https://learn.microsoft.com/en-us/windows/win32/api/memoryapi/nf-memoryapi-virtualalloc2 - I'm hoping it can be used interchangeably with VirtualAlloc, but not familiar with win32 programming (for 32-bit)

[mvorisek]

immediately freeing it with VirtualFree

it is a "my custom debug log" from zend_alloc, so there are/were some php ops between

My best idea for a workaround is to retry the entire request for aligned memory

so this is thankfully not needed

0x000001e7 Win error

comes from zend_mm_mmap_fixed with given != null ptr, but ptr - ptr+size is not avaialble, easy to reproduce on many test runs /w x86 builds

linux has same problem, free may be called on data php did not allocate

I am understanding this problem, so I will soon, then we can discuss if there is anything non-bug related to improve

each process's virtual memory space is distinct, so it probably isn't the action of other processes

memory addresses are randomized even within a single process, and some addresses are reserved for dll, exe itself, ... there are definitely some addresses taken

[cmb69]

I did some more debugging on x86 Win build:

Thank you! (Although I don't think that 32bit Windows builds are important; does anybody still use them?)

alloc: 0CE00000, 00000000, 200000

That looks fishy. Why would we want to allocate zero bytes in the first place? And I wouldn't be surprised that nothing is allocated in this case, so trying to free should fail.

[mvorisek]

Thank you! (Although I don't think that 32bit Windows builds are important; does anybody still use them?)

I do not use 32 bit builds ATM neither, but I belive 32 bit builds are important to discover issues like these, which are present also on 64 bit builds but are much harder to debug because they are much more rare.

alloc: 0CE00000, 00000000, 200000

see "syntax is", the middle value is result of VirtualAlloc, all ok, we only need to handle it :)

[mvorisek]

what looks a little fishy to me is 0x3ff000 size allocation, there is some aligment in the zend_alloc, the original allocation size is 0x200000, but then I would expect (same nicely rounded) 0x400000 value, is that intended?

[TysonAndre]

what looks a little fishy to me is 0x3ff000 size allocation, there is some aligment in the zend_alloc, the original allocation size is 0x200000, but then I would expect (same nicely rounded) 0x400000 value, is that intended?

Yes, that's deliberate - a block of memory of size 2MB + 2MB - 4kb (size + alignment - REAL_PAGE_SIZE is guaranteed to have one region that's of size exactly 2MB if the allocator aligns to the 4kb page size

		/* chunk has to be aligned */
		zend_mm_munmap(ptr, size);
		ptr = zend_mm_mmap(size + alignment - REAL_PAGE_SIZE);

[dstogov]

I have split the commit into two - one for easy to check/follow refactoring and then commit with the actual functional changes.

If it's possible to fix a problem with 1 screen diff. It doesn't make a lot of sense to do the same with 10 times bigger patch. (introducing functions used once, reordering them and so on). The less - the better, especial if we target this into an old stable version and don't like to introduce new problems.

[mvorisek]

@dstogov for Windows the allocation logic is changed and improved, for example we do not try to allocate the size first in hope it will be aligned, because in almost all cases it is not aligned, we also do not need the problematic code with race condition for x64 (for x86 it is there too utilize the virtual address space more dense)

the static methods for Windows are better then using VirtualAlloc directly as errors need to be handled and free is more complex as we need to query the allocation base address first

so the proposed changes should be about minimal

[dstogov]

@mvorisek in case your patch would clean, I would approve it on last week. Today I spent an hour trying to read it, understand all the changes and don't miss anything. I failed. I understand, it's clear for you, because you already worked on it for a while, but for others it would be easier to rewrite this (without noise).

• logic improvement and re-factoring are good for development branch
• it's important to keep the readable history of changes for core sub systems
• agree, it's better to move the first allocation attempt in zend_mm_chunk_alloc_int() under #ifndef _WIN32
• I don't know why static methods are better for Windows (if they perform syscalls anyway)
• I think, it's better to handle WIN32 and WIN64 in the same way (I'm not completely sure why WIN64 should be handled differently)
• we don't need VirtualQuery. I suppose it comes from an attempt of reusing old code in zend_mm_chunk_alloc_int() or over-design
• I don't care about single-line changes in error messages

I may still misunderstand some details of your patch

[mvorisek]

logic improvement and re-factoring are good for development branch

I did the refactoring as the new fixed code can be then reviewed much more easier.

The 1st commit can be reviewed by moving the methods locally in text editor and then comparing the result. There are very few changes actually.

I don't know why static methods are better for Windows (if they perform syscalls anyway)

There are static methods: zend_mm_mmap_win_reserve, zend_mm_mmap_win_reserve_fixed, zend_mm_mmap_win_commit for consitency with static methods like zend_mm_mmap for linux.

zend_mm_mmap_win_reserve is even used more than once.

I think, it's better to handle WIN32 and WIN64 in the same way (I'm not completely sure why WIN64 should be handled differently)

It is handled the same, only on x86 we try allocation subject to race condition first. If it fails, we continue with unified x64/x86 allocation.

we don't need VirtualQuery. I suppose it comes from an attempt of reusing old code in zend_mm_chunk_alloc_int() or over-design

It is needed, as the commited memory start can be different vs. the reserved memory start. To release all/reserved memory, we need to query VirtualQuery.

Please review the commits separately, the 1st one is a preparation for the 2nd one, 1st one can be reviewed easily, 2nd one should be obvious quickly.

[dstogov]

I understand that this VirtualQuery() is need to handle unaligned reserved blocks, but it's better to reserve aligned block in first place. I don't know where you find this trick. Is it recommended by MSDN?

[mvorisek]

I came to the solution after huge research, all Windows memory allocators are wrappers around VirtualAlloc/VirtualFree/VirtualQuery API

[mvorisek]

I have reworked this PR to be a minimalistic bugfix and a fallback solution only when #9650 is hit.

When #9650 is hit, the allocated memory might be up to the alignment size larger, but such problem is hit in about 0.01% alloc calls on x86 Windows only, so it should be fine.

For easier review, please follow the individual commits, if possible, please keep the commits unsquashed when merging.

[dstogov]

The patch might be smaller, but now it's readable and looks right.
(I didn't test, just looked over the code).

[dstogov]

I'm not sure if this check is necessary, because you already request the fixed aligned segment. Anyway, this is not a big problem.

[mvorisek]

removed - good catch, this was there originally, but it will never be executed, also there is ZEND_ASSERT(ptr == addr) newly in zend_mm_mmap_fixed

[dstogov]

It may make sense to call VirtualFree() first and than fall-back to VirtualQuery()+VirtualFree() like mi_os_mem_free().
See https://github.com/microsoft/mimalloc/blob/v2.0.6/src/os.c#L359..L372

[mvorisek]

I am aware they do it this way, but I query VirtualQuery always to prevent the modification of global error code (err).

[dstogov]

Now the allocation of "unaligned" block occurs as a fall-back for case when aligned block can't be allocated. I suspect we will have more aligned blocks then unaligned and therefore single VirualFree() for the most often case might be better.

[mvorisek]

done

[dstogov]

@cmb69 could you please take a final look and merge this

[cmb69]

Thank you all!