Skip to content
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

Closed

Conversation

brandonpayton
Copy link
Member

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.

@brandonpayton brandonpayton added [Type] Bug An existing feature does not function as intended [Feature] PHP.wasm Mission-critical labels Apr 10, 2024
@brandonpayton brandonpayton requested a review from a team April 10, 2024 03:23
@brandonpayton brandonpayton self-assigned this Apr 10, 2024
@brandonpayton
Copy link
Member Author

Note: This uses the following PHP instrumentation: brandonpayton/php-src#2

@brandonpayton
Copy link
Member Author

brandonpayton commented Apr 10, 2024

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:

 FAIL  src/lib/steps/import-wxr.spec.ts > Blueprint step importWxr > Should import a WXR file with JSON-encoded UTF-8 characters
       Error: memory access out of bounds 

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.

@brandonpayton
Copy link
Member Author

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:

FAIL  src/lib/steps/import-wxr.spec.ts > Blueprint step importWxr > Should import a WXR file with JSON-encoded UTF-8 characters
...
Caused by: RuntimeError: memory access out of bounds        
 ❯ dlfree ../../../wasm:/wasm/033df742:1:35097                            
 ❯ _str_dtor ../../../wasm:/wasm/033df742:1:6649956
 ❯ zend_hash_destroy ../../../wasm:/wasm/033df742:1:57354
 ❯ zend_interned_strings_deactivate ../../../wasm:/wasm/033df742:1:5374537
 ❯ call ../../../wasm:/wasm/033df742:1:4209829
 ❯ ret.<computed> ../../php-wasm/node/public/php_8_0.js:6682:24
 ❯ runtime.asm.<computed> ../../php-wasm/universal/src/lib/wasm-error-reporting.ts:45:13
 ❯ invoke_v ../../php-wasm/node/public/php_8_0.js:7605:3
 ❯ php_request_shutdown ../../../wasm:/wasm/033df742:1:1702048
 ❯ wasm_sapi_request_shutdown ../../../wasm:/wasm/033df742:1:3234276
Caused by: Error: 
 ❯ Object.Asyncify.handleSleep ../../php-wasm/node/public/php_8_0.js:7771:49
 ❯ _wasm_poll_socket ../../php-wasm/node/public/php_8_0.js:6543:18
 ❯ php_pollfd_for ../../../wasm:/wasm/033df742:1:597125
 ❯ php_network_connect_socket ../../../wasm:/wasm/033df742:1:3745501
 ❯ php_tcp_sockop_set_option ../../../wasm:/wasm/033df742:1:5800009
 ❯ php_openssl_sockop_set_option ../../../wasm:/wasm/033df742:1:5844119
 ❯ _php_stream_set_option ../../../wasm:/wasm/033df742:1:227674
 ❯ dynCall_iiiii ../../../wasm:/wasm/033df742:1:6483311
 ❯ ret.<computed> ../../php-wasm/node/public/php_8_0.js:6682:24
 ❯ runtime.asm.<computed> ../../php-wasm/universal/src/lib/wasm-error-reporting.ts:45:13

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.

@brandonpayton
Copy link
Member Author

The latest did encounter a "memory access out of bounds" error here.

The most interesting bits are:

Serialized Error: {
  "betterMessage": "memory access out of bounds",
}
Caused by: RuntimeError: memory access out of bounds
 ❯ zend_mm_gc ../../../wasm:/wasm/033df742:1:1235311
 ❯ zend_mm_alloc_huge ../../../wasm:/wasm/033df742:1:1812884
 ❯ _emalloc ../../../wasm:/wasm/033df742:1:36343
 ❯ zend_string_tolower_ex ../../../wasm:/wasm/033df742:1:161488
 ❯ zend_compile_var_inner ../../../wasm:/wasm/033df742:1:522334
 ❯ zend_compile_expr_inner ../../../wasm:/wasm/033df742:1:96236
 ❯ zend_compile_expr_inner ../../../wasm:/wasm/033df742:1:100273
 ❯ zend_compile_stmt ../../../wasm:/wasm/033df742:1:577164
 ❯ zend_compile_stmt ../../../wasm:/wasm/033df742:1:570044
 ❯ zend_compile_stmt ../../../wasm:/wasm/033df742:1:576544

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:

  • zend_mm_chunk_alloc()
  • zend_mm_chunk_free()
  • zend_mm_chunk_truncate()
  • zend_mm_chunk_extend()

@brandonpayton brandonpayton mentioned this pull request Apr 10, 2024
@adamziel
Copy link
Collaborator

FWIW E2e failures are unrelated to this PR

@brandonpayton
Copy link
Member Author

FWIW E2e failures are unrelated to this PR

Thanks, @adamziel! Closing this because a fix has been merged in #1229.

@brandonpayton brandonpayton deleted the investigate-oob-test-with-instrumented-code branch April 11, 2024 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] PHP.wasm Mission-critical [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PHP memory leak
2 participants