Skip to content

Commit

Permalink
Avoid partial munmap memory leak (#1189)
Browse files Browse the repository at this point in the history
Adds custom PHP memory storage handlers to stop PHP from
attempting to partial unmap memory via the `munmap()` function.
Emscripten allocators do not support using `munmap()` to partially unmap
memory. They fail for partial unmaps, and when attempts fail, memory is
leaked because PHP thinks the memory is free while Emscripten libs
consider it allocated.

The custom memory storage handlers take over responsibility for
allocating and freeing aligned memory chunks. The handlers use
`posix_memalign()` and `free()` instead of `mmap()` and `munmap()`, and
this appears to resolve the problem.

The handlers are added as part of a PHP extension, which is not perfect
because allocation can happen before the extension is initialized. But
the amount of allocation before the workaround is applied should be
quite small.

## Testing Instructions

Before applying this patch:
Run [this test
script](https://gist.github.com/brandonpayton/934d96d76b96557e94ad6983aaffa09f)
and verify that it OOMs.

After applying this patch:
Rerun the test script and verify that it completes after 1000
interations.

## Remaining work

- [x] Compare performance with php-wasm builds prior to this patch and
reconsider if 20% slower.
- [x] Find out whether it is sufficient to add the workaround during the
PHP module init phase rather than during each request init
- [x] Address minor TODO comments in wasm_memory_storage.c
- [x] See if we can run the test script as part of tests under
`packages/php-wasm/node/src/test`
  • Loading branch information
brandonpayton authored Apr 4, 2024
1 parent b8686b9 commit b38ae63
Show file tree
Hide file tree
Showing 63 changed files with 379 additions and 27 deletions.
41 changes: 41 additions & 0 deletions packages/php-wasm/compile/php-wasm-memory-storage/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
*.lo
*.la
.libs
acinclude.m4
aclocal.m4
autom4te.cache
build
config.guess
config.h
config.h.in
config.log
config.nice
config.status
config.sub
configure
configure.ac
configure.in
include
install-sh
libtool
ltmain.sh
Makefile
Makefile.fragments
Makefile.global
Makefile.objects
missing
mkinstalldirs
modules
php_test_results_*.txt
phpt.*
run-test-info.php
run-tests.php
tests/**/*.diff
tests/**/*.out
tests/**/*.php
tests/**/*.exp
tests/**/*.log
tests/**/*.sh
tests/**/*.db
tests/**/*.mem
tmp-php.ini
94 changes: 94 additions & 0 deletions packages/php-wasm/compile/php-wasm-memory-storage/config.m4
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
dnl config.m4 for extension wasm_memory_storage

dnl Comments in this file start with the string 'dnl'.
dnl Remove where necessary.

dnl If your extension references something external, use 'with':

dnl PHP_ARG_WITH([wasm_memory_storage],
dnl [for wasm_memory_storage support],
dnl [AS_HELP_STRING([--with-wasm_memory_storage],
dnl [Include wasm_memory_storage support])])

dnl Otherwise use 'enable':

PHP_ARG_ENABLE([wasm_memory_storage],
[whether to enable wasm_memory_storage support],
[AS_HELP_STRING([--enable-wasm_memory_storage],
[Enable wasm_memory_storage support])],
[no])

if test "$PHP_WASM_MEMORY_STORAGE" != "no"; then
dnl Write more examples of tests here...

dnl Remove this code block if the library does not support pkg-config.
dnl PKG_CHECK_MODULES([LIBFOO], [foo])
dnl PHP_EVAL_INCLINE($LIBFOO_CFLAGS)
dnl PHP_EVAL_LIBLINE($LIBFOO_LIBS, WASM_MEMORY_STORAGE_SHARED_LIBADD)

dnl If you need to check for a particular library version using PKG_CHECK_MODULES,
dnl you can use comparison operators. For example:
dnl PKG_CHECK_MODULES([LIBFOO], [foo >= 1.2.3])
dnl PKG_CHECK_MODULES([LIBFOO], [foo < 3.4])
dnl PKG_CHECK_MODULES([LIBFOO], [foo = 1.2.3])

dnl Remove this code block if the library supports pkg-config.
dnl --with-wasm_memory_storage -> check with-path
dnl SEARCH_PATH="/usr/local /usr" # you might want to change this
dnl SEARCH_FOR="/include/wasm_memory_storage.h" # you most likely want to change this
dnl if test -r $PHP_WASM_MEMORY_STORAGE/$SEARCH_FOR; then # path given as parameter
dnl WASM_MEMORY_STORAGE_DIR=$PHP_WASM_MEMORY_STORAGE
dnl else # search default path list
dnl AC_MSG_CHECKING([for wasm_memory_storage files in default path])
dnl for i in $SEARCH_PATH ; do
dnl if test -r $i/$SEARCH_FOR; then
dnl WASM_MEMORY_STORAGE_DIR=$i
dnl AC_MSG_RESULT(found in $i)
dnl fi
dnl done
dnl fi
dnl
dnl if test -z "$WASM_MEMORY_STORAGE_DIR"; then
dnl AC_MSG_RESULT([not found])
dnl AC_MSG_ERROR([Please reinstall the wasm_memory_storage distribution])
dnl fi

dnl Remove this code block if the library supports pkg-config.
dnl --with-wasm_memory_storage -> add include path
dnl PHP_ADD_INCLUDE($WASM_MEMORY_STORAGE_DIR/include)

dnl Remove this code block if the library supports pkg-config.
dnl --with-wasm_memory_storage -> check for lib and symbol presence
dnl LIBNAME=WASM_MEMORY_STORAGE # you may want to change this
dnl LIBSYMBOL=WASM_MEMORY_STORAGE # you most likely want to change this

dnl If you need to check for a particular library function (e.g. a conditional
dnl or version-dependent feature) and you are using pkg-config:
dnl PHP_CHECK_LIBRARY($LIBNAME, $LIBSYMBOL,
dnl [
dnl AC_DEFINE(HAVE_WASM_MEMORY_STORAGE_FEATURE, 1, [ ])
dnl ],[
dnl AC_MSG_ERROR([FEATURE not supported by your wasm_memory_storage library.])
dnl ], [
dnl $LIBFOO_LIBS
dnl ])

dnl If you need to check for a particular library function (e.g. a conditional
dnl or version-dependent feature) and you are not using pkg-config:
dnl PHP_CHECK_LIBRARY($LIBNAME, $LIBSYMBOL,
dnl [
dnl PHP_ADD_LIBRARY_WITH_PATH($LIBNAME, $WASM_MEMORY_STORAGE_DIR/$PHP_LIBDIR, WASM_MEMORY_STORAGE_SHARED_LIBADD)
dnl AC_DEFINE(HAVE_WASM_MEMORY_STORAGE_FEATURE, 1, [ ])
dnl ],[
dnl AC_MSG_ERROR([FEATURE not supported by your wasm_memory_storage library.])
dnl ],[
dnl -L$WASM_MEMORY_STORAGE_DIR/$PHP_LIBDIR -lm
dnl ])
dnl
dnl PHP_SUBST(WASM_MEMORY_STORAGE_SHARED_LIBADD)

dnl In case of no dependencies
AC_DEFINE(HAVE_WASM_MEMORY_STORAGE, 1, [ Have wasm_memory_storage support ])

PHP_NEW_EXTENSION(wasm_memory_storage, wasm_memory_storage.c, $ext_shared)
fi
7 changes: 7 additions & 0 deletions packages/php-wasm/compile/php-wasm-memory-storage/config.w32
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
ARG_ENABLE('wasm_memory_storage', 'wasm_memory_storage support', 'no');

if (PHP_WASM_MEMORY_STORAGE != 'no') {
AC_DEFINE('HAVE_WASM_MEMORY_STORAGE', 1, 'wasm_memory_storage support enabled');

EXTENSION('wasm_memory_storage', 'wasm_memory_storage.c', null, '/DZEND_ENABLE_STATIC_TSRMLS_CACHE=1');
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/* wasm_memory_storage extension for PHP */

#ifndef PHP_WASM_MEMORY_STORAGE_H
# define PHP_WASM_MEMORY_STORAGE_H

extern zend_module_entry wasm_memory_storage_module_entry;
# define phpext_wasm_memory_storage_ptr &wasm_memory_storage_module_entry

# define PHP_WASM_MEMORY_STORAGE_VERSION "0.0.1"

#endif /* PHP_WASM_MEMORY_STORAGE_H */
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
/**
* wasm_memory_storage extension for PHP.
*
* The purpose of this extension is to work around a memory leak caused by
* failing attempts to partially unmap memory allocated with mmap().
* By providing custom memory storage, we can avoid mmap()/munmap() calls and
* use posix_memalign()/free() instead.
*
* Background:
* Issue: https://github.com/WordPress/wordpress-playground/issues/1128
* PR: https://github.com/WordPress/wordpress-playground/pull/1189
*/

#ifdef HAVE_CONFIG_H
# include "config.h"
#endif

#include <stdio.h>
#include "php.h"
#include "ext/standard/info.h"
#include "Zend/zend_alloc.h"
#include "php_wasm_memory_storage.h"

/**
* Allocate a chunk of memory.
*
* This function implements the PHP Zend custom memory storage operation "chunk_free()".
*
* Here is an example offered in the PHP source code:
* https://github.com/php/php-src/blob/dbaeb62ab1e34067057170ab50cf39d1bde584d8/Zend/zend_alloc.h#L325
*
* @param storage The zend_mm_storage struct for this allocation.
* @param size The number of bytes to allocate.
* @param alignment The byte alignment of the memory allocation.
* @returns A pointer to allocated memory or NULL on failure.
*/
void* wasm_memory_storage_chunk_alloc(zend_mm_storage* storage, size_t size, size_t alignment)
{
void* ptr = NULL;
if (posix_memalign(&ptr, alignment, size) == 0)
{
return ptr;
} else {
return NULL;
}
}

/**
* Free a chunk of memory.
*
* This function implements the PHP Zend custom memory storage operation "chunk_free()".
*
* Here is an example offered in the PHP source code:
* https://github.com/php/php-src/blob/dbaeb62ab1e34067057170ab50cf39d1bde584d8/Zend/zend_alloc.h#L352
*
* @param storage The zend_mm_storage struct for this memory.
* @param ptr The pointer to the memory to free.
* @param size The size of the memory to free. Ignored.
* @returns void
*/
void wasm_memory_storage_chunk_free(zend_mm_storage* storage, void* ptr, size_t size)
{
free(ptr);
}

zend_mm_storage wasm_memory_storage_struct = {
.handlers = {
.chunk_alloc = &wasm_memory_storage_chunk_alloc,
.chunk_free = &wasm_memory_storage_chunk_free,
.chunk_truncate = NULL,
.chunk_extend = NULL,
},
.data = NULL,
};

// These definitions are mirrored from zend_alloc.c:
// https://github.com/php/php-src/blob/dbaeb62ab1e34067057170ab50cf39d1bde584d8/Zend/zend_alloc.c#L131-L137
#ifndef ZEND_MM_CUSTOM
# define ZEND_MM_CUSTOM 1 /* support for custom memory allocator */
#endif
#ifndef ZEND_MM_STORAGE
# define ZEND_MM_STORAGE 1 /* support for custom memory storage */
#endif

// This struct mirrors the heap structure, which is declared privately
// in zend_alloc.c. We use this to more easily and clearly assign our custom memory storage handler.
// https://github.com/php/php-src/blob/dbaeb62ab1e34067057170ab50cf39d1bde584d8/Zend/zend_alloc.c#L234-L240
typedef struct _wasm_memory_storage_heap {
#if ZEND_MM_CUSTOM
int custom;
#endif
#if ZEND_MM_STORAGE
zend_mm_storage *storage;
#endif
} wasm_memory_storage_heap;

PHP_MINIT_FUNCTION(wasm_memory_storage)
{
wasm_memory_storage_heap* heap = (wasm_memory_storage_heap*) zend_mm_get_heap();
heap->storage = &wasm_memory_storage_struct;

return SUCCESS;
}

PHP_MSHUTDOWN_FUNCTION(wasm_memory_storage)
{
wasm_memory_storage_heap* heap = (wasm_memory_storage_heap*) zend_mm_get_heap();
heap->storage = NULL;

return SUCCESS;
}

/* {{{ PHP_MINFO_FUNCTION */
PHP_MINFO_FUNCTION(wasm_memory_storage)
{
php_info_print_table_start();
php_info_print_table_row(2, "wasm_memory_storage support", "enabled");
php_info_print_table_end();
}
/* }}} */

/* {{{ wasm_memory_storage_module_entry */
zend_module_entry wasm_memory_storage_module_entry = {
STANDARD_MODULE_HEADER,
"wasm_memory_storage", /* Extension name */
NULL, /* zend_function_entry */
PHP_MINIT(wasm_memory_storage), /* PHP_MINIT - Module initialization */
PHP_MSHUTDOWN(wasm_memory_storage), /* PHP_MSHUTDOWN - Module shutdown */
NULL, /* PHP_RINIT - Request initialization */
NULL, /* PHP_RSHUTDOWN - Request shutdown */
PHP_MINFO(wasm_memory_storage), /* PHP_MINFO - Module info */
PHP_WASM_MEMORY_STORAGE_VERSION, /* Version */
STANDARD_MODULE_PROPERTIES
};
/* }}} */
8 changes: 8 additions & 0 deletions packages/php-wasm/compile/php/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ RUN git clone https://github.com/php/php-src.git php-src \
--single-branch \
--depth 1;

# Work around memory leak due to PHP using Emscripten's incomplete mmap/munmap support
COPY ./php-wasm-memory-storage /root/php-src/ext/wasm_memory_storage

RUN cd php-src && ./buildconf --force

# Bring in the libraries
Expand Down Expand Up @@ -214,6 +217,10 @@ RUN if [ "$WITH_MBREGEX" = "yes" ] && [ "${PHP_VERSION:0:3}" != "7.0" ]; \
COPY ./php/php*.patch /root/
RUN cd /root && \
git apply --no-index /root/php${PHP_VERSION:0:3}*.patch -v && \
( [[ "${PHP_VERSION:0:3}" == '8.3' ]] && \
git apply --no-index /root/php-chunk-alloc-zend-assert-8.3.patch -v || \
git apply --no-index /root/php-chunk-alloc-zend-assert.patch -v \
) && \
touch php-src/patched

# Add VRZNO if needed
Expand Down Expand Up @@ -270,6 +277,7 @@ RUN source /root/emsdk/emsdk_env.sh && \
--enable-bcmath \
--enable-ctype \
--enable-tokenizer \
--enable-wasm_memory_storage \
$(cat /root/.php-configure-flags)

# Silence the errors "munmap() failed: [28] Invalid argument"
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
diff --git a/php-src/Zend/zend_alloc.c b/php-src/Zend/zend_alloc.c
index bf2116fc91..bec65453d8 100644
--- a/php-src/Zend/zend_alloc.c
+++ b/php-src/Zend/zend_alloc.c
@@ -773,7 +773,7 @@ static void *zend_mm_chunk_alloc(zend_mm_heap *heap, size_t size, size_t alignme
#if ZEND_MM_STORAGE
if (UNEXPECTED(heap->storage)) {
void *ptr = heap->storage->handlers.chunk_alloc(heap->storage, size, alignment);
- ZEND_ASSERT(((uintptr_t)((char*)ptr + (alignment-1)) & (alignment-1)) == (uintptr_t)ptr);
+ ZEND_ASSERT(((uintptr_t)((char*)ptr + (alignment-1)) & ~(alignment-1)) == (uintptr_t)ptr);
return ptr;
}
#endif
13 changes: 13 additions & 0 deletions packages/php-wasm/compile/php/php-chunk-alloc-zend-assert.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
diff --git a/php-src/Zend/zend_alloc.c b/php-src/Zend/zend_alloc.c
index bf2116fc91..bec65453d8 100644
--- a/php-src/Zend/zend_alloc.c
+++ b/php-src/Zend/zend_alloc.c
@@ -773,7 +773,7 @@ static void *zend_mm_chunk_alloc(zend_mm_heap *heap, size_t size, size_t alignme
#if ZEND_MM_STORAGE
if (UNEXPECTED(heap->storage)) {
void *ptr = heap->storage->handlers.chunk_alloc(heap->storage, size, alignment);
- ZEND_ASSERT(((zend_uintptr_t)((char*)ptr + (alignment-1)) & (alignment-1)) == (zend_uintptr_t)ptr);
+ ZEND_ASSERT(((zend_uintptr_t)((char*)ptr + (alignment-1)) & ~(alignment-1)) == (zend_uintptr_t)ptr);
return ptr;
}
#endif
Binary file modified packages/php-wasm/node/public/7_0_33/php_7_0.wasm
Binary file not shown.
Binary file modified packages/php-wasm/node/public/7_1_30/php_7_1.wasm
Binary file not shown.
Binary file modified packages/php-wasm/node/public/7_2_34/php_7_2.wasm
Binary file not shown.
Binary file modified packages/php-wasm/node/public/7_3_33/php_7_3.wasm
Binary file not shown.
Binary file modified packages/php-wasm/node/public/7_4_33/php_7_4.wasm
Binary file not shown.
Binary file modified packages/php-wasm/node/public/8_0_30/php_8_0.wasm
Binary file not shown.
Binary file modified packages/php-wasm/node/public/8_1_23/php_8_1.wasm
Binary file not shown.
Binary file modified packages/php-wasm/node/public/8_2_10/php_8_2.wasm
Binary file not shown.
Binary file modified packages/php-wasm/node/public/8_3_0/php_8_3.wasm
Binary file not shown.
2 changes: 1 addition & 1 deletion packages/php-wasm/node/public/php_7_0.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
const dependencyFilename = __dirname + '/7_0_33/php_7_0.wasm';
export { dependencyFilename };
export const dependenciesTotalSize = 12778149;
export const dependenciesTotalSize = 12779191;
export function init(RuntimeName, PHPLoader) {
/**
* Overrides Emscripten's default ExitStatus object which gets
Expand Down
2 changes: 1 addition & 1 deletion packages/php-wasm/node/public/php_7_1.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
const dependencyFilename = __dirname + '/7_1_30/php_7_1.wasm';
export { dependencyFilename };
export const dependenciesTotalSize = 13300133;
export const dependenciesTotalSize = 13301166;
export function init(RuntimeName, PHPLoader) {
/**
* Overrides Emscripten's default ExitStatus object which gets
Expand Down
2 changes: 1 addition & 1 deletion packages/php-wasm/node/public/php_7_2.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
const dependencyFilename = __dirname + '/7_2_34/php_7_2.wasm';
export { dependencyFilename };
export const dependenciesTotalSize = 13991154;
export const dependenciesTotalSize = 13991791;
export function init(RuntimeName, PHPLoader) {
/**
* Overrides Emscripten's default ExitStatus object which gets
Expand Down
2 changes: 1 addition & 1 deletion packages/php-wasm/node/public/php_7_3.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
const dependencyFilename = __dirname + '/7_3_33/php_7_3.wasm';
export { dependencyFilename };
export const dependenciesTotalSize = 14097593;
export const dependenciesTotalSize = 14098602;
export function init(RuntimeName, PHPLoader) {
/**
* Overrides Emscripten's default ExitStatus object which gets
Expand Down
2 changes: 1 addition & 1 deletion packages/php-wasm/node/public/php_7_4.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
const dependencyFilename = __dirname + '/7_4_33/php_7_4.wasm';
export { dependencyFilename };
export const dependenciesTotalSize = 14330034;
export const dependenciesTotalSize = 14331003;
export function init(RuntimeName, PHPLoader) {
/**
* Overrides Emscripten's default ExitStatus object which gets
Expand Down
2 changes: 1 addition & 1 deletion packages/php-wasm/node/public/php_8_0.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
const dependencyFilename = __dirname + '/8_0_30/php_8_0.wasm';
export { dependencyFilename };
export const dependenciesTotalSize = 13597144;
export const dependenciesTotalSize = 13597807;
export function init(RuntimeName, PHPLoader) {
/**
* Overrides Emscripten's default ExitStatus object which gets
Expand Down
Loading

0 comments on commit b38ae63

Please sign in to comment.