Fix a debug assertion in externref garbage collections
#3734
Conversation
| // table. So we need to ensure that this `externref` is in the table | ||
| // *before* we GC, even though `insert_with_gc` will ensure that it is in | ||
| // the table *after* the GC. | ||
| activations_table.insert_without_gc(externref.clone()); |
There was a problem hiding this comment.
This can end up doing a double-insertion between this and insert_with_gc below, and it might be good to avoid that? This method is only called when the table is full so insert_without_gc is already guaranteed to go straight to insertion in the slow path, so should a specialized method of the table be added for basically this use case?
There was a problem hiding this comment.
It won't insert into the bump chunk, so I don't think there is anything to worry about here. We just do one more hash lookup than is strictly necessary, but that is fine as this is a slow path anyways.
There was a problem hiding this comment.
That's true, yeah. Could you update the comment here to indicate that this is doing some extra work but it's the slow path so it should be ok?
github-actions
bot
added
fuzzing
When we GC, we assert the invariant that all `externref`s we find on the stack have a corresponding entry in the `VMExternRefActivationsTable`. However, we also might be in code that is in the process of fixing up this invariant and adding an entry to the table, but the table's bump chunk is full, and so we do a GC and then add the entry into the table. This will ultimately maintain our desired invariant, but there is a moment in time when we are doing the GC where the invariant is relaxed which is okay because the reference will be in the table before we return to Wasm or do anything else. This isn't a possible UAF, in other words. To make it so that the assertion won't trip, we explicitly insert the reference into the table *before* we GC, so that the invariant is not relaxed across a possibly-GCing operation (even though it would be safe in this particular case).
fitzgen
force-pushed
the
externref-debug-assertion-fix
branch
from
3fcfe12
to
ac10725
Compare
Subscribe to Label Actioncc @fitzgen
This issue or pull request has been labeled: "fuzzing", "wasmtime:ref-types"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
This will make it generate `table.set`s that come from `global.get`s and `global.get`s that come from `table.set`s. Ultimately, it should give us much more fuzzing coverage of `externref` globals, their barriers, and passing `externref`s into and out of Wasm to get or set globals.
fitzgen
force-pushed
the
externref-debug-assertion-fix
branch
from
ac10725
to
f292ff5
Compare
| // table. So we need to ensure that this `externref` is in the table | ||
| // *before* we GC, even though `insert_with_gc` will ensure that it is in | ||
| // the table *after* the GC. | ||
| activations_table.insert_without_gc(externref.clone()); |
There was a problem hiding this comment.
That's true, yeah. Could you update the comment here to indicate that this is doing some extra work but it's the slow path so it should be ok?
See commit messages for details.