Stream: git-wasmtime

Topic: wasmtime / PR #10456 Fix needs-stack-map propagation for ...

Wasmtime GitHub notifications bot (Mar 21 2025 at 21:29):

fitzgen requested abrown for a review on PR #10456.

Wasmtime GitHub notifications bot (Mar 21 2025 at 21:29):

fitzgen requested wasmtime-compiler-reviewers for a review on PR #10456.

Wasmtime GitHub notifications bot (Mar 21 2025 at 21:29):

fitzgen requested wasmtime-core-reviewers for a review on PR #10456.

Wasmtime GitHub notifications bot (Mar 21 2025 at 21:29):

fitzgen opened PR #10456 from fitzgen:issue-10397 to bytecodealliance:main:

We were previously propagating the needs-inclusion-in-stack-maps bit from a variable to the values resulting from builder.use_var(my_var) and passed to builder.def_var(my_var, my_val). However, that does not cover every ir::Value generated for a given ir::Variable: it is missing block parameters that the SSA builder inserted and which are not then used directly as the result of use_var, but instead passed as arguments another block, and that block's parameter is then directly or indirectly used as the result of builder.use_var(...). This chaining can go arbitrarily deep, based on the program's control flow. By not marking these block parameters as needing inclusion in stack maps, our stack maps would be incomplete, which meant that the GC could too-eagerly collect objects, which meant that we would then get use-after-free bugs.

The solution is for the SSABuilder to report all the block parameters that it inserted, and for which ir::Variable it inserted them. We already have a summary of the mutations that an SSABuilder performed, SideEffects, and which must be handled by the FunctionBuilder after, e.g., sealing a block, so we add this block-param-and-var information to it. Now, after the FunctionBuilder has the SSABuilder do its thing, it handles fixing up not only the modified blocks' status, which it was doing before, but also propagates the needs-inclusion-in-stack-maps metadata from variables to all those freshly-inserted block parameter values.

After this change, we successfully include all of a needs-inclusion-in-stack-maps variable's values in stack maps, which avoids the over-eager object reclamation and associated GC heap corruption.

Fixes #10397

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Mar 21 2025 at 21:29):

fitzgen requested pchickey for a review on PR #10456.

Wasmtime GitHub notifications bot (Mar 21 2025 at 21:48):

fitzgen edited PR #10456:

We were previously propagating the needs-inclusion-in-stack-maps bit from a variable to the values resulting from builder.use_var(my_var) and passed to builder.def_var(my_var, my_val). However, that does not cover every ir::Value generated for a given ir::Variable: it is missing block parameters that the SSA builder inserted and which are not then used directly as the result of use_var, but instead passed as arguments another block, and that block's parameter is then directly or indirectly used as the result of builder.use_var(...). This chaining can go arbitrarily deep, based on the program's control flow. By not marking these block parameters as needing inclusion in stack maps, our stack maps would be incomplete, which meant that the GC could too-eagerly collect objects, which meant that we would then get use-after-free bugs.

The solution is for the SSABuilder to report all the block parameters that it inserted, and for which ir::Variable it inserted them. We already have a summary of the mutations that an SSABuilder performed, SideEffects, and which must be handled by the FunctionBuilder after, e.g., sealing a block, so we add this block-param-and-var information to it. Now, after the FunctionBuilder has the SSABuilder do its thing, it handles fixing up not only the modified blocks' status, which it was doing before, but also propagates the needs-inclusion-in-stack-maps metadata from variables to all those freshly-inserted block parameter values.

After this change, we successfully include all of a needs-inclusion-in-stack-maps variable's values in stack maps, which avoids the over-eager object reclamation and associated GC heap corruption.

Fixes #10397
Fixes #10398

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Mar 21 2025 at 22:00):

fitzgen edited PR #10456:

We were previously propagating the needs-inclusion-in-stack-maps bit from a variable to the values resulting from builder.use_var(my_var) and passed to builder.def_var(my_var, my_val). However, that does not cover every ir::Value generated for a given ir::Variable: it is missing block parameters that the SSA builder inserted and which are not then used directly as the result of use_var, but instead passed as arguments another block, and that block's parameter is then directly or indirectly used as the result of builder.use_var(...). This chaining can go arbitrarily deep, based on the program's control flow. By not marking these block parameters as needing inclusion in stack maps, our stack maps would be incomplete, which meant that the GC could too-eagerly collect objects, which meant that we would then get use-after-free bugs.

The solution is for the SSABuilder to report all the block parameters that it inserted, and for which ir::Variable it inserted them. We already have a summary of the mutations that an SSABuilder performed, SideEffects, and which must be handled by the FunctionBuilder after, e.g., sealing a block, so we add this block-param-and-var information to it. Now, after the FunctionBuilder has the SSABuilder do its thing, it handles fixing up not only the modified blocks' status, which it was doing before, but also propagates the needs-inclusion-in-stack-maps metadata from variables to all those freshly-inserted block parameter values.

After this change, we successfully include all of a needs-inclusion-in-stack-maps variable's values in stack maps, which avoids the over-eager object reclamation and associated GC heap corruption.

Fixes #10397
Fixes #10398
Fixes #10399

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Mar 21 2025 at 22:03):

fitzgen edited PR #10456:

We were previously propagating the needs-inclusion-in-stack-maps bit from a variable to the values resulting from builder.use_var(my_var) and passed to builder.def_var(my_var, my_val). However, that does not cover every ir::Value generated for a given ir::Variable: it is missing block parameters that the SSA builder inserted and which are not then used directly as the result of use_var, but instead passed as arguments another block, and that block's parameter is then directly or indirectly used as the result of builder.use_var(...). This chaining can go arbitrarily deep, based on the program's control flow. By not marking these block parameters as needing inclusion in stack maps, our stack maps would be incomplete, which meant that the GC could too-eagerly collect objects, which meant that we would then get use-after-free GC heap corruption bugs (still limited to within the GC heap because of our sandboxing approach) that would ultimately become (safe) panics when runtime code operated on corrupted objects.

The solution is for the SSABuilder to report all the block parameters that it inserted, and for which ir::Variable it inserted them. We already have a summary of the mutations that an SSABuilder performed, SideEffects, and which must be handled by the FunctionBuilder after, e.g., sealing a block, so we add this block-param-and-var information to it. Now, after the FunctionBuilder has the SSABuilder do its thing, it handles fixing up not only the modified blocks' status, which it was doing before, but also propagates the needs-inclusion-in-stack-maps metadata from variables to all those freshly-inserted block parameter values.

After this change, we successfully include all of a needs-inclusion-in-stack-maps variable's values in stack maps, which avoids the over-eager object reclamation and associated GC heap corruption.

Fixes #10397
Fixes #10398
Fixes #10399

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Mar 21 2025 at 23:44):

github-actions[bot] commented on PR #10456:

Subscribe to Label Action

cc @fitzgen

<details>
This issue or pull request has been labeled: "cranelift", "wasmtime:api", "wasmtime:ref-types"

Thus the following users have been cc'd because of the following labels:

• fitzgen: wasmtime:ref-types

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Mar 24 2025 at 18:14):

cfallin submitted PR review:

LGTM -- thanks for the thorough unit test!

Wasmtime GitHub notifications bot (Mar 24 2025 at 18:37):

cfallin merged PR #10456.

Last updated: Apr 17 2025 at 06:04 UTC