Wasmtime GitHub notifications bot (Feb 09 2024 at 14:42): Wasmtime GitHub notifications bot (Feb 09 2024 at 14:42):alexcrichton requested fitzgen for a review on PR #7902.
Wasmtime GitHub notifications bot (Feb 09 2024 at 14:42):alexcrichton opened PR #7902 from alexcrichton:more-typechecks to bytecodealliance:main:
Right now the abstraction for host-owned resources in Wasmtime is quite simple, it's "just an index". This can be error prone because the host can suffer both from use-after-free and ABA-style problems. While there's not much that can be done about use-after-free the previous implementation would accidentally enable "AB" style problems where if a host-owned resource index was deallocated and then reallocated with another type the original handle would still work. While not a major bug this can be confusing and additionally violate's our guarantees as a component runtime to guests to ensure that resources always have valid
reps going into components.This commit adds a new layer of storage which keeps track of the
ResourceTypefor all host-owned resources. This means that resources going into a host-owned table now record their type and coming out of a host-owned table a typecheck is always performed. Note that guests can continue to skip this logic as they already have per-type tables and so won't suffer the same issue.This change is inspired by my taking a look at #7883. The crux of the issue was a typo where a resource was reused by accident which led to confusing behavior due to the reuse. This change instead makes it return an error more quickly and doesn't allow the component to see any invalid state.
Closes #7883
<!--
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.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
Wasmtime GitHub notifications bot (Feb 09 2024 at 14:42):alexcrichton requested wasmtime-core-reviewers for a review on PR #7902.
Wasmtime GitHub notifications bot (Feb 09 2024 at 15:09):alexcrichton updated PR #7902.
Wasmtime GitHub notifications bot (Feb 09 2024 at 15:10):alexcrichton updated PR #7902.
Wasmtime GitHub notifications bot (Feb 09 2024 at 16:44):github-actions[bot] commented on PR #7902:
Subscribe to Label Action
cc @peterhuene
<details>
This issue or pull request has been labeled: "wasmtime:api"Thus the following users have been cc'd because of the following labels:
- peterhuene: wasmtime:api
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 (Feb 09 2024 at 16:51):fitzgen commented on PR #7902:
I haven't looked at the actual PR yet, just read your comment, but:
Is recording the type info enough to solve the ABA problem?
I don't think so: we could allocate a resource of type A, create an index handle to it, deallocate it, allocate a new resource that is also of type A in that place, and then use the old index handle to access the wrong resource.
I think adding a generation counter is the most straightforward way to definitely resolve the ABA problem here.
Wasmtime GitHub notifications bot (Feb 09 2024 at 17:25):fitzgen submitted PR review:
The code itself LGTM, and this is an improvement over the current status quo, so I don't mind landing this.
But I'd like to hear your thoughts on how we can integrate a generation to fully prevent ABA issues. I'm not 100% sure on the constraints we are working within here. For example, are we restricted to
u32indices and therefore we can't add a secondu32for generation? If so, perhaps we can repurpose 8 bits into a generation? Would 24 bits be enough for all active resources? Do we have a limit on how many active resources can exist at a time? Such a small generation likely wouldn't be enough to fully prevent ABA problems, but I think it would still be a further improvement.
Wasmtime GitHub notifications bot (Feb 09 2024 at 17:25):fitzgen submitted PR review:
The code itself LGTM, and this is an improvement over the current status quo, so I don't mind landing this.
But I'd like to hear your thoughts on how we can integrate a generation to fully prevent ABA issues. I'm not 100% sure on the constraints we are working within here. For example, are we restricted to
u32indices and therefore we can't add a secondu32for generation? If so, perhaps we can repurpose 8 bits into a generation? Would 24 bits be enough for all active resources? Do we have a limit on how many active resources can exist at a time? Such a small generation likely wouldn't be enough to fully prevent ABA problems, but I think it would still be a further improvement.
Wasmtime GitHub notifications bot (Feb 09 2024 at 17:25):fitzgen created PR review comment:
commented out code
Wasmtime GitHub notifications bot (Feb 09 2024 at 17:25):fitzgen created PR review comment:
todo
Wasmtime GitHub notifications bot (Feb 12 2024 at 20:01):alexcrichton updated PR #7902.
Wasmtime GitHub notifications bot (Feb 12 2024 at 20:03):alexcrichton commented on PR #7902:
A few days ago I was halfway through writing up a comment saying that I wasn't originally intending to solve the "full ABA problem" but was just targetting a smaller slice. Halfway through I asked myself "but why?" and scrapped that approach and took your suggestion of the generation counters and such.
I think this should be an even stronger guarantee now where host-owned resources should be nearly fully resistant to ABA issues (modulo overflow of counters)
Mind taking another look though @fitzgen since enough has changed?
Wasmtime GitHub notifications bot (Feb 12 2024 at 22:07):fitzgen submitted PR review:
Nice!
Wasmtime GitHub notifications bot (Feb 12 2024 at 22:07):fitzgen submitted PR review:
Nice!
Wasmtime GitHub notifications bot (Feb 12 2024 at 22:07):fitzgen created PR review comment:
Mind explicitly masking off the generation and doing
u32::try_from(masked).unwrap()? I think that would make things a lot more clear and avoid hiding subtle issues withascasts as we change representations and stuff like that.
Wasmtime GitHub notifications bot (Feb 12 2024 at 22:16):alexcrichton updated PR #7902.
Wasmtime GitHub notifications bot (Feb 12 2024 at 22:16):alexcrichton has enabled auto merge for PR #7902.
Wasmtime GitHub notifications bot (Feb 12 2024 at 23:11):alexcrichton merged PR #7902.
Last updated: Nov 22 2024 at 17:03 UTC