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

fix: bindgen trappable_errors using unversion/versioned packages #8305

Merged
merged 1 commit into from
Apr 5, 2024

Conversation

fibonacci1729
Copy link
Contributor

This PR fixes a bug in the wasmtime::bindgen! macro when specifying trappable_errors and there is a versioned and unversioned representation of a package.

@fibonacci1729
Copy link
Contributor Author

cc/ @alexcrichton

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Thanks for this!

tests/spec_testsuite Outdated Show resolved Hide resolved
@fibonacci1729 fibonacci1729 force-pushed the fix-bindgen-bug branch 2 times, most recently from 4f4ab17 to 0816456 Compare April 5, 2024 19:04
@fibonacci1729 fibonacci1729 marked this pull request as ready for review April 5, 2024 19:05
@fibonacci1729 fibonacci1729 requested a review from a team as a code owner April 5, 2024 19:05
@fibonacci1729 fibonacci1729 requested review from fitzgen and removed request for a team April 5, 2024 19:05
Signed-off-by: Brian H <brian.hardock@fermyon.com>
@alexcrichton alexcrichton added this pull request to the merge queue Apr 5, 2024
Merged via the queue into bytecodealliance:main with commit 82284a3 Apr 5, 2024
19 checks passed
alexcrichton pushed a commit to alexcrichton/wasmtime that referenced this pull request Apr 5, 2024
alexcrichton pushed a commit to alexcrichton/wasmtime that referenced this pull request Apr 5, 2024
@alexcrichton
Copy link
Member

I've opened backports for this at #8306 and #8307

alexcrichton added a commit that referenced this pull request Apr 10, 2024
…) (#8306)

Signed-off-by: Brian H <brian.hardock@fermyon.com>
Co-authored-by: Brian <brian.hardock@fermyon.com>
alexcrichton added a commit that referenced this pull request Apr 10, 2024
…) (#8307)

Signed-off-by: Brian H <brian.hardock@fermyon.com>
Co-authored-by: Brian <brian.hardock@fermyon.com>
alexcrichton pushed a commit to alexcrichton/wasmtime that referenced this pull request Apr 11, 2024
alexcrichton added a commit that referenced this pull request Apr 11, 2024
* cranelift: Include clobbers and outgoing args in stack limit (#8301)

When we compute the amount of space that we need in a stack frame for
the stack limit check, we were only counting spill-slots and explicit
stack-slots. However, we need to account for all uses of the stack which
occur before the next stack limit check. That includes clobbers and any
stack arguments we want to pass to callees.

The maximum amount that we could have missed by is essentially bounded
by the number of arguments which could be passed to a function. In
Wasmtime, that is limited by `MAX_WASM_FUNCTION_PARAMS` in
`wasmparser::limits`, which is set to 1,000, and the largest arguments
are 16-byte vectors, so this could undercount by about 16kB.

This is not a security issue according to Wasmtime's security policy
(https://docs.wasmtime.dev/security-what-is-considered-a-security-vulnerability.html)
because it's the embedder's responsibility to ensure that the stack
where Wasmtime is running has enough extra space on top of the
configured `max_wasm_stack` size, and getting within 16kB of the host
stack size is too small to be safe even with this fixed.

However, this was definitely not the intended behavior when stack limit
checks or stack probes are enabled, and anyone with non-default
configurations or non-Wasmtime uses of Cranelift should evaluate whether
this bug impacts your use case.

(For reference: When Wasmtime is used in async mode or on Linux, the
default stack size is 1.5MB larger than the default WebAssembly stack
limit, so such configurations are typically safe regardless. On the
other hand, on macOS the default non-async stack size for threads other
than the main thread is the same size as the default for
`max_wasm_stack`, so that is too small with or without this bug fix.)

* fix: bindgen trappable_errors using unversion/versioned packages (#8305)

Signed-off-by: Brian H <brian.hardock@fermyon.com>

* Cranelift: Do not dedupe/GVN bitcasts from reference values (#8317)

* Cranelift: Do not dedupe/GVN bitcasts from reference values

Deduping bitcasts to integers from references can make the references no long
longer live across safepoints, and instead only the bitcasted integer results
would be. Because the reference is no longer live after the safepoint, the
safepoint's stack map would not have an entry for the reference, which could
result in the collector reclaiming an object too early, which is basically a
use-after-free bug. Luckily, we sandbox the GC heap now, so such UAF bugs aren't
memory unsafe, but they could potentially result in denial of service
attacks. Either way, we don't want those bugs!

On the other hand, it is technically fine to dedupe bitcasts *to* reference
types. Doing so extends, rather than shortens, the live range of the GC
reference. This potentially adds it to more stack maps than it otherwise would
have been in, which means it might unnecessarily survive a GC it otherwise
wouldn't have. But that is fine. Shrinking live ranges of GC references, and
removing them from stack maps they otherwise should have been in, is the
problematic transformation.

* Add additional logging and debug asserts for GC stuff

* Handle out-of-bounds component sections (#8323)

* Handle out-of-bounds component sections

Fixes #8322

* Add a test that trancated component binaries don't cause panics

---------

Signed-off-by: Brian H <brian.hardock@fermyon.com>
Co-authored-by: Jamey Sharp <jsharp@fastly.com>
Co-authored-by: Brian <brian.hardock@fermyon.com>
Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants