Fix use-after-free resolving reference type names in wast-parser - #2805
Conversation
|
Should we have |
Move the truncation of resolve_ref_types_/resolve_type_vectors_/ resolve_funcs_ out of ParseModuleFieldList and into ParseModuleField, so a field that fails to parse drops its own deferred resolutions instead of the caller tracking them. The dispatch switch moves to ParseModuleFieldImpl unchanged; ParseModuleField now records the list sizes, calls it, and truncates on failure. No behavioural change: the six registration sites still exit 1 with only the expected parse diagnostics and no ASan report.
|
Agreed, that's better. Moved it into The dispatch switch moved into Re-checked after the change: all six registration sites (func param, func result, func local, table elem type, elem segment type, import func signature) exit 1 with only the expected parse diagnostics and no ASan report, and reverting just the truncation brings the heap-use-after-free straight back in |
| resolve_type_vectors_.begin() + type_vectors_size, | ||
| resolve_type_vectors_.end()); | ||
| resolve_funcs_.erase(resolve_funcs_.begin() + funcs_size, | ||
| resolve_funcs_.end()); |
There was a problem hiding this comment.
I'm still a little confused, if we are returning failure, when why does it matter about the state of these members? If we fail to parse why is anybody accessing these after this point?
There was a problem hiding this comment.
The failure doesn't propagate. ParseModuleFieldList swallows it:
if (Failed(ParseModuleField(module))) {
CHECK_RESULT(Synchronize(IsModuleField));
}It resynchronises to the next module field and carries on parsing, so wat2wasm reports every parse error in one pass rather than stopping at the first. The failed field's result is dropped right there.
So control still reaches the three resolve loops at the bottom of that same function, which walk resolve_ref_types_ / resolve_type_vectors_ / resolve_funcs_ and dereference the pointers recorded while the field was being parsed. Those point into the field's local unique_ptr, which was destroyed on the error path instead of being handed to AppendField. That's the freed read, and a freed write via local_types.Set() in the func-local case.
The one case where nobody touches them is when Synchronize fails too, i.e. the bad field is the last thing in the file, since then we return early and never reach the loops. That's why the test has a (memory 1) after the broken fields: it gives Synchronize somewhere to land so the loops are actually reached.
2 participants
drop the deferred reference-type resolutions a module field registered before it failed to parse, otherwise ParseModuleFieldList dereferences pointers into a field that was destroyed instead of appended.