-
Notifications
You must be signed in to change notification settings - Fork 268
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.
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
Investigate OOB: Run unit tests with instrumented PHP 8.0 code #1220
Conversation
This reverts commit a30405b.
Based on instrumentation here: https://github.com/brandonpayton/php-src/tree/php-8-instrumented specifically: brandonpayton/php-src@c9e3618
Note: This uses the following PHP instrumentation: brandonpayton/php-src#2 |
This latest CI run has a lot more "memory access out of bounds" errors. Either I made a mistake putting together this PR or something relevant changed on trunk since we started testing this. Running tests locally shows a single out of bounds failure:
That may be good news. We weren't able to reproduce this locally at all before . Maybe we can associate it with a recent change to guess why this might be happening. Something to look at in the morning. |
I went ahead and removed some of the PHP instrumentation because we are seeing a crash in a different place now. The first test after that passed except for an unnecessary failure in the OOM test where it checks for stderr output. I disabled that here and plan to remove it line on trunk later. Locally, I'm seeing this "memory access out of bounds" crash and call stack:
Note the dlfree() call there. Looks like it's happening during shutdown. I am looking at how we can patch PHP to just always use posix_memalign() to see if we can get some consistency and stop crashing. |
The latest did encounter a "memory access out of bounds" error here. The most interesting bits are:
So now we're seeing crashes that involve actual memory management functions. I don't know for sure, but it seems like this must have to do with mixing approaches to alloc and free operations. There are indeed allocations that happen before wasm_memory_storage is put into place, and PHP may be assuming it can use the wasm_memory_storage functions to free() memory that was allocated a different way. As mentioned above, I'm planning to look at patching PHP memory allocation more directly to address this issue, specifically the following functions from zend_alloc.c:
|
…repro" This reverts commit 86162c1.
FWIW E2e failures are unrelated to this PR |
This PR investigates a "memory access out of bounds" error we see when running with the wasm_memory_storage extension to work around a serious memory leak. The purpose is to fix #1128 soundly.
Let's keep these investigation PRs focused so each is trying one thing. We can leave them all open until the issue is resolved.
The main reason for creating this PR is to reflect work already done under #1199 (which we closed because there were too many different things tried under a single PR).
The next thing I'd like to try to actually resolve this is to directly patch PHP's zend_mm_init() function to use wasm_memory_storage immediately here.