Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Clear WASM linear memory on other OSes besides Linux too when using fast instance reuse #10291

Merged

Conversation

koute
Copy link
Contributor

@koute koute commented Nov 17, 2021

Fixes #10095

Originally I intended to just disable fast instance reuse when the --import-memory flag is not specified when compiling the runtime, however it seems we do already always clear the linear memory anyway (only on Linux) as a side-effect of #8998 so it seems to make more sense to just make this behavior official and apply to other OSes too?

This might have a performance impact on non-Linux OSes, however:

  1. There are relatively easy ways to make it faster (either use another OS-specific API if available, or just spawn a dedicated memory clearing thread to do this work in the background; on modern systems clearing memory is really fast, so it's just an issue of doing in asynchronously to reduce latency)
  2. Having a core behavior like this be different between OSes may possibly invite even more weird bugs in the future (Hyrum's law and whatnot)

@koute koute requested review from pepyakin and bkchr November 17, 2021 07:14
@koute koute added A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit B0-silent Changes should not be mentioned in any release notes labels Nov 17, 2021
@pepyakin
Copy link
Contributor

JFYI, for №1 for macOS there was this PR #9164

Copy link
Contributor

@pepyakin pepyakin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we are officially bringing back the original memory zeroing guarantee, huh

@koute
Copy link
Contributor Author

koute commented Nov 18, 2021

bot merge

@paritytech-processbot paritytech-processbot bot merged commit e670c04 into paritytech:master Nov 18, 2021
grishasobol pushed a commit to gear-tech/substrate that referenced this pull request Mar 28, 2022
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wasm trap: out of bounds memory access
3 participants