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

Memory out of bound error #169

Closed
ZafarKamal123 opened this issue Mar 23, 2023 · 11 comments
Closed

Memory out of bound error #169

ZafarKamal123 opened this issue Mar 23, 2023 · 11 comments
Labels
[Priority] High [Type] Bug An existing feature does not function as intended

Comments

@ZafarKamal123
Copy link

ZafarKamal123 commented Mar 23, 2023

The following memory access error occurs on the official playground of WordPress WASM when trying to publish a page with a lot of content (not really a lot):

Screenshot 2023-03-23 at 4 58 26 PM

Steps to Reproduce

  1. Navigate to the wasm playground demo.
  2. Add a lot of content in the gutenberg editor
  3. Try to publish the code

Expected Behaviour

It should publish the page successfully.

@adamziel adamziel added [Type] Bug An existing feature does not function as intended [Priority] High labels Mar 23, 2023
@adamziel
Copy link
Collaborator

Thank you for reporting! This is a legitimate bug. The message talks about memory, but luckily it’s unrelated to actual memory consumption. You’re not running out of memory. Rather, PHP is trying to read something from the wrong memory region. It should be a relatively easy fix, I'll look into that.

@ZafarKamal123
Copy link
Author

Thanks @adamziel for the quick response. Looking forward for the fix as i'm willing to use it into my project.

@adamziel
Copy link
Collaborator

Update: Here's an unminified stacktrace:

Uncaught (in promise) RuntimeError: memory access out of bounds
    at zend_activate_auto_globals (01b77c16)
    at php_hash_environment (01b77c16)
    at invoke_i (php_8_0.js:7636:36)
    at php_request_startup (01b77c16)
    at wasm_sapi_request_init (01b77c16)
    at invoke_i (php_8_0.js:7636:36)
    at wasm_sapi_handle_request (01b77c16)
    at php_8_0.js:914:22
    at Object.ccall (php_8_0.js:6999:22)
    at #handleRequest (php.ts:697:34)

@adamziel
Copy link
Collaborator

adamziel commented Mar 24, 2023

Something's wrong with the way POST body is read. Presumably memcpy writes to/from a region it's not supposed to:

#if PHP_MAJOR_VERSION == 5
static int wasm_sapi_read_post_body(char *buffer, uint count_bytes)
#else
static size_t wasm_sapi_read_post_body(char *buffer, size_t count_bytes)
#endif
{
	if (wasm_server_context == NULL || wasm_server_context->request_body == NULL)
	{
		return 0;
	}

	count_bytes = MIN(count_bytes, SG(request_info).content_length - SG(read_post_bytes));
	if(count_bytes > 0) {
		memcpy(buffer + SG(read_post_bytes), wasm_server_context->request_body + SG(read_post_bytes), count_bytes);
	}
	return count_bytes;
}

@adamziel
Copy link
Collaborator

Note to myself: compare that logic to php-fpm SAPI https://github.com/php/php-src/blob/master/sapi/fpm/fpm/fpm_main.c

@adamziel
Copy link
Collaborator

Php-fcgi has interesting safeguards in place:

static size_t sapi_fcgi_read_post(char *buffer, size_t count_bytes)
{
	size_t read_bytes = 0;
	int tmp_read_bytes;
	fcgi_request *request = (fcgi_request*) SG(server_context);
	size_t remaining = SG(request_info).content_length - SG(read_post_bytes);

	if (remaining < count_bytes) {
		count_bytes = remaining;
	}
	while (read_bytes < count_bytes) {
		size_t diff = count_bytes - read_bytes;
		int to_read = (diff > INT_MAX) ? INT_MAX : (int)diff;

		tmp_read_bytes = fcgi_read(request, buffer + read_bytes, to_read);
		if (tmp_read_bytes <= 0) {
			break;
		}
		read_bytes += tmp_read_bytes;
	}
	return read_bytes;
}

@adamziel
Copy link
Collaborator

adamziel commented Mar 24, 2023

Hypothesis: if POST body exceeds SAPI_POST_BLOCK_SIZE, or about 16k bytes, the POST-reading loop runs for the second time - which is what trigger the error. If so, this is probably the buffer is of the SAPI_POST_BLOCK_SIZE size and is reset every time. However, the post reader assumes it must memcpy to buffer + SG(read_post_bytes) which not only leaves the buffer empty but also corrupts the memory region after if. I’ll verify this when I have a moment.

@adamziel
Copy link
Collaborator

adamziel commented Mar 25, 2023

That was it! I'm rebuilding PHP and will ship a fix shortly.

@adamziel adamziel reopened this Mar 25, 2023
@adamziel
Copy link
Collaborator

Fix shipped in 2e950a2

I still need to deploy the updated version to wasm.wordpress.net, but this issue may be closed. @ZafarKamal123 can you confirm this worked? If not, I'm happy to reopen.

@ZafarKamal123
Copy link
Author

Thanks for the quick support man! I'll check this and get back to you.

@adamziel
Copy link
Collaborator

It’s now deployed to wasm.wordpress.net! The editor is still experiencing a few issues due to #174, but not this one.

adamziel added a commit that referenced this issue Mar 29, 2023
…API_POST_BLOCK_SIZE

Fixes #169

If POST body exceeds SAPI_POST_BLOCK_SIZE, or about 16k bytes, the POST-reading loop runs
for the second time - which is what trigger the error. If so, this is probably the buffer
is of the SAPI_POST_BLOCK_SIZE size and is reset every time. However, the post reader assumes
it must memcpy to buffer + SG(read_post_bytes) which not only leaves the buffer empty but also
corrupts the memory region after if.

This PR writes to the buffer directly instead of moving the pointer by SG(read_post_bytes)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Priority] High [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

2 participants