alexcrichton requested fitzgen for a review on PR #7902.
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
rep
s going into components.This commit adds a new layer of storage which keeps track of the
ResourceType
for 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
-->
alexcrichton requested wasmtime-core-reviewers for a review on PR #7902.
alexcrichton updated PR #7902.
alexcrichton updated PR #7902.
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>
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.
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
u32
indices and therefore we can't add a secondu32
for 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.
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
u32
indices and therefore we can't add a secondu32
for 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.
fitzgen created PR review comment:
commented out code
fitzgen created PR review comment:
todo
alexcrichton updated PR #7902.
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?
fitzgen submitted PR review:
Nice!
fitzgen submitted PR review:
Nice!
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 withas
casts as we change representations and stuff like that.
alexcrichton updated PR #7902.
alexcrichton has enabled auto merge for PR #7902.
alexcrichton merged PR #7902.
Last updated: Dec 23 2024 at 12:05 UTC