Skip to content

Fix zend_alloc aligned allocation on Windows #9721

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 6 commits into from
Closed

Fix zend_alloc aligned allocation on Windows #9721

Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0736b8a
dedup munmap call /w error handling
mvorisek Nov 12, 2022
8b92113
add error handling for fixed Windows alloc
mvorisek Nov 12, 2022
d8d8fd9
fix log alloc errors only once
mvorisek Nov 12, 2022
93b71b8
Fix fixed addr range alloc is not free for Windows
mvorisek Nov 12, 2022
e6ea363
remove never reached code
mvorisek Nov 14, 2022
817fd19
call VirtualQuery only if VirtualFree failed first
mvorisek Nov 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 63 additions & 44 deletions Zend/zend_alloc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,11 +416,49 @@ stderr_last_error(char *msg)
/* OS Allocation */
/*****************/

static void zend_mm_munmap(void *addr, size_t size)
{
#ifdef _WIN32
MEMORY_BASIC_INFORMATION mbi;
if (VirtualQuery(addr, &mbi, sizeof(mbi)) == 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

#if ZEND_MM_ERROR
stderr_last_error("VirtualQuery() failed");
#endif
}
addr = mbi.AllocationBase;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (VirtualFree(addr, 0, MEM_RELEASE) == 0) {
#if ZEND_MM_ERROR
stderr_last_error("VirtualFree() failed");
#endif
}
#else
if (munmap(addr, size) != 0) {
#if ZEND_MM_ERROR
fprintf(stderr, "\nmunmap() failed: [%d] %s\n", errno, strerror(errno));
#endif
}
#endif
}

#ifndef HAVE_MREMAP
static void *zend_mm_mmap_fixed(void *addr, size_t size)
{
#ifdef _WIN32
return VirtualAlloc(addr, size, MEM_COMMIT | MEM_RESERVE, PAGE_READWRITE);
void *ptr = VirtualAlloc(addr, size, MEM_COMMIT | MEM_RESERVE, PAGE_READWRITE);

if (ptr == NULL) {
/** ERROR_INVALID_ADDRESS is expected when fixed addr range is not free */
if (GetLastError() != ERROR_INVALID_ADDRESS) {
#if ZEND_MM_ERROR
stderr_last_error("VirtualAlloc() fixed failed");
#endif
}
SetLastError(0);
return NULL;
}
ZEND_ASSERT(ptr == addr);
return ptr;
#else
int flags = MAP_PRIVATE | MAP_ANON;
#if defined(MAP_EXCL)
Expand All @@ -431,15 +469,11 @@ static void *zend_mm_mmap_fixed(void *addr, size_t size)

if (ptr == MAP_FAILED) {
#if ZEND_MM_ERROR && !defined(MAP_EXCL)
fprintf(stderr, "\nmmap() failed: [%d] %s\n", errno, strerror(errno));
fprintf(stderr, "\nmmap() fixed failed: [%d] %s\n", errno, strerror(errno));
#endif
return NULL;
} else if (ptr != addr) {
if (munmap(ptr, size) != 0) {
#if ZEND_MM_ERROR
fprintf(stderr, "\nmunmap() failed: [%d] %s\n", errno, strerror(errno));
#endif
}
zend_mm_munmap(ptr, size);
return NULL;
}
return ptr;
Expand Down Expand Up @@ -483,23 +517,6 @@ static void *zend_mm_mmap(size_t size)
#endif
}

static void zend_mm_munmap(void *addr, size_t size)
{
#ifdef _WIN32
if (VirtualFree(addr, 0, MEM_RELEASE) == 0) {
#if ZEND_MM_ERROR
stderr_last_error("VirtualFree() failed");
#endif
}
#else
if (munmap(addr, size) != 0) {
#if ZEND_MM_ERROR
fprintf(stderr, "\nmunmap() failed: [%d] %s\n", errno, strerror(errno));
#endif
}
#endif
}

/***********/
/* Bitmask */
/***********/
Expand Down Expand Up @@ -682,14 +699,28 @@ static void *zend_mm_chunk_alloc_int(size_t size, size_t alignment)
zend_mm_munmap(ptr, size);
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);
ptr = zend_mm_mmap_fixed((void*)((char*)ptr + (alignment - offset)), size);
offset = ZEND_MM_ALIGNED_OFFSET(ptr, alignment);
if (offset != 0) {
zend_mm_munmap(ptr, size);
return NULL;
offset = alignment - offset;
}
zend_mm_munmap(ptr, size + alignment - REAL_PAGE_SIZE);
ptr = zend_mm_mmap_fixed((void*)((char*)ptr + offset), size);
if (ptr == NULL) { // fix GH-9650, fixed addr range is not free
ptr = zend_mm_mmap(size + alignment - REAL_PAGE_SIZE);
if (ptr == NULL) {
return NULL;
}
offset = ZEND_MM_ALIGNED_OFFSET(ptr, alignment);
if (offset != 0) {
ptr = (void*)((char*)ptr + alignment - offset);
}
} else {
offset = ZEND_MM_ALIGNED_OFFSET(ptr, alignment);
if (offset != 0) {
zend_mm_munmap(ptr, size);
return NULL;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
return ptr;
#else
offset = ZEND_MM_ALIGNED_OFFSET(ptr, alignment);
Expand Down Expand Up @@ -1847,11 +1878,7 @@ static zend_mm_heap *zend_mm_init(void)

if (UNEXPECTED(chunk == NULL)) {
#if ZEND_MM_ERROR
#ifdef _WIN32
stderr_last_error("Can't initialize heap");
#else
fprintf(stderr, "\nCan't initialize heap: [%d] %s\n", errno, strerror(errno));
#endif
fprintf(stderr, "Can't initialize heap\n");
#endif
return NULL;
}
Expand Down Expand Up @@ -2978,11 +3005,7 @@ ZEND_API zend_mm_heap *zend_mm_startup_ex(const zend_mm_handlers *handlers, void
chunk = (zend_mm_chunk*)handlers->chunk_alloc(&tmp_storage, ZEND_MM_CHUNK_SIZE, ZEND_MM_CHUNK_SIZE);
if (UNEXPECTED(chunk == NULL)) {
#if ZEND_MM_ERROR
#ifdef _WIN32
stderr_last_error("Can't initialize heap");
#else
fprintf(stderr, "\nCan't initialize heap: [%d] %s\n", errno, strerror(errno));
#endif
fprintf(stderr, "Can't initialize heap\n");
#endif
return NULL;
}
Expand Down Expand Up @@ -3025,11 +3048,7 @@ ZEND_API zend_mm_heap *zend_mm_startup_ex(const zend_mm_handlers *handlers, void
if (!storage) {
handlers->chunk_free(&tmp_storage, chunk, ZEND_MM_CHUNK_SIZE);
#if ZEND_MM_ERROR
#ifdef _WIN32
stderr_last_error("Can't initialize heap");
#else
fprintf(stderr, "\nCan't initialize heap: [%d] %s\n", errno, strerror(errno));
#endif
fprintf(stderr, "Can't initialize heap\n");
#endif
return NULL;
}
Expand Down