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

Stack Overflow: memory access out of bounds #816

Closed
swissspidy opened this issue Nov 28, 2023 · 5 comments
Closed

Stack Overflow: memory access out of bounds #816

swissspidy opened this issue Nov 28, 2023 · 5 comments
Labels
[Feature] PHP.wasm [Priority] High [Type] Bug An existing feature does not function as intended

Comments

@swissspidy
Copy link
Member

swissspidy commented Nov 28, 2023

Fix: #870


This might be a reincarnation of #169

To reproduce:

  1. Open https://playground.wordpress.net/?plugin=web-stories&php-extension-bundle=kitchen-sink
  2. Go to Dashboard -> Stories
  3. Hover over a template and click on "Use template"
  4. This would normally trigger a REST API POST request with a (relatively large) body
  5. See memory access out of bounds error in console

Are there some body size limits with playground that one should be aware of?

@adamziel
Copy link
Collaborator

Possibly related: #416

Are there some body size limits with playground that one should be aware of?

Not that I know of, other than the PHP memory limit which should result in a PHP Fatal Error, not wasm crash. That's definitely a bug in Playground. Some memcpy call must be missing a boundary check, or something to that effect.

@adamziel adamziel added [Type] Bug An existing feature does not function as intended [Priority] High [Feature] PHP.wasm labels Nov 28, 2023
@seanmorris seanmorris self-assigned this Dec 7, 2023
@seanmorris
Copy link
Contributor

This is the same bug as #870, and is fixed by the same code.

@adamziel
Copy link
Collaborator

adamziel commented Dec 20, 2023

Potentially related Emscripten issue: Automatically growing the stack. Passing a large request body or response body via stack may be hitting the default stack size of, I think, 1MB.

@adamziel
Copy link
Collaborator

adamziel commented Jan 29, 2024

The issue is indeed a related to stack size. Clicking "Use template" calls wasm_set_request_body with a body string that's 153KB large which is too much for the current WASM build. #870 explores a potential fix by storing the request body on HEAP:

		const size = this[__private__dont__use].lengthBytesUTF8(body);
		const addr = this.malloc(size + 1);
		this[__private__dont__use].stringToUTF8(body, addr, size + 1);
		this[__private__dont__use].ccall(
			'wasm_set_request_body',
			null,
			[NUMBER],
			[addr]
		);

I'm not convinced about encoding the body bytes as UTF8 so the details may change, but there's a good chance the heap approach would fix this problem.

@adamziel adamziel changed the title memory access out of bounds Stack Overflow: memory access out of bounds Jan 29, 2024
adamziel added a commit that referenced this issue Feb 2, 2024
Before this PR, requests with large body trigger a stack overflow error.
For example, issue #816 describes how clicking a "Use template" button
calls wasm_set_request_body with a body string that's 153KB large which
is too much for the current WASM build.

This PR fixes the issue by allocating the request body on the heap, not
on the stack.

 ## Implementation details

`BasePHP.setRequestBody` converts the body string into bytes using
the `stringToUTF8` function provided by Emscripten. This is just as
it was before this PR. This method isn't perfect and will break for
some inputs. The `setRequestBody` method should just accept a `UInt8Array`
instead of a string, but that's outside of scope of this PR.

 ## Testing instructions

Confirm the CI checks are green. This PR ships with a series of checks
to confirm the crash is fixed.

For manual testing, navigate to https://beemovie.fandom.com/wiki/Bee_Movie/Transcript and Select All, then Copy the selection.

Then go to http://localhost:5400/website-server/?php=8.3&wp=6.4&storage=none&url=/wp-admin/post-new.php
and paste the content of the clipboard directly into the editor. The javascript will take a moment to digest
the pasted content into blocks. The editor should end up creating very many blocks. Once thats done, click
publish, and the post should save without crashing.
adamziel added a commit that referenced this issue Feb 2, 2024
Before this PR, requests with large body trigger a stack overflow error.
For example, issue #816 describes how clicking a "Use template" button
calls wasm_set_request_body with a body string that's 153KB large which
is too much for the current WASM build.

This PR fixes the issue by allocating the request body on the heap, not
on the stack.

## Implementation details

`BasePHP.setRequestBody` allocates enough heap memory to store the body
string. Then, it converts the body string into bytes and writes it into
heap. Once the request is finished, `BasePHP.run()` frees that memory.

The conversion to bytes is done via the `stringToUTF8` function provided
by Emscripten. This is just as it was before this PR. This method isn't
perfect and will break for some inputs. The `setRequestBody` method
should just accept a `UInt8Array` instead of a string, but that's
outside of scope of this PR.

## Testing instructions

Confirm the CI checks are green. This PR ships with a series of checks
to confirm the crash is fixed.

For manual testing, navigate to
https://beemovie.fandom.com/wiki/Bee_Movie/Transcript and Select All,
then Copy the selection.

Then go to
http://localhost:5400/website-server/?php=8.3&wp=6.4&storage=none&url=/wp-admin/post-new.php
and paste the content of the clipboard directly into the editor. The
javascript will take a moment to digest the pasted content into blocks.
The editor should end up creating very many blocks. Once thats done,
click publish, and the post should save without crashing.

Supersedes #870
@adamziel
Copy link
Collaborator

adamziel commented Feb 2, 2024

@swissspidy this should be fixed! :-) Thank you for reporting, please let me know about any other issues you stumble upon.

CleanShot 2024-02-02 at 15 35 17@2x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] PHP.wasm [Priority] High [Type] Bug An existing feature does not function as intended
Projects
No open projects
Development

No branches or pull requests

3 participants