Perform stronger typechecks of host-owned resources #7902
Conversation
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 bytecodealliance#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 bytecodealliance#7883
alexcrichton
force-pushed
the
more-typechecks
branch
from
acbf642 to
b245988
Compare
github-actions
bot
added
the
wasmtime:api
Subscribe to Label Actioncc @peterhuene
This issue or pull request has been labeled: "wasmtime:api"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
|
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. |
There was a problem hiding this comment.
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 second u32 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.
|
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? |
| fn index(&self) -> u32 { | ||
| self.0 as u32 | ||
| } |
There was a problem hiding this 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 with as casts as we change representations and stuff like that.
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