Disallow uses of MWasmDerived{,Index}Pointer with reftyped-bases
Categories
(Core :: JavaScript: WebAssembly, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox111 | --- | fixed |
People
(Reporter: jseward, Assigned: jseward)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
MWasmDerivedPointer denotes a base-plus-constant-offset value. In the
presence of wasm gc-types, this is dangerous when the base value is reftyped.
This is because MWasmDerivedPointer makes it possible to construct values that
we have an obligation to modify at GC time, yet it does not give us any
mechanism to do so, since they are not valid object pointers from the GC's
point of view. MWasmDerivedIndexPointer is similarly dangerous.
Hence if these values are live across a GC, they will be invalid after the GC,
and hence cause a crash if used after the GC. This has been observed to
happen.
A reasonable fix seems to be to restrict their base types to be non-ref. For
cases where the base type is ref, instead pass around a pair of the base and
the (constant) offset, to the place where the access actually happens. In
effect what this does is to make it impossible to create these "sort-of but not
really a ref" values.
See https://phabricator.services.mozilla.com/D166004#5458408 and
https://phabricator.services.mozilla.com/D166004#5458908.
Assignee | ||
Comment 1•1 year ago
|
||
MWasmDerivedPointer denotes a base-plus-constant-offset value. In the
presence of wasm-gc types, this is dangerous when the base value is reftyped.
This is because MWasmDerivedPointer makes it possible to construct values that
we have an obligation to modify at GC time, yet it does not give us any
mechanism to do so, since they are not valid object pointers from the GC's
point of view. MWasmDerivedIndexPointer is similarly dangerous.
Hence if these values are live across a GC, they will be invalid after the GC,
and hence cause a crash if used after the GC. This has been observed to
happen.
A reasonable fix seems to be to restrict their base types to be non-ref. For
cases where the base type is ref, instead pass around a pair of the base and
the (constant) offset, to the place where the access actually happens. In
effect what this does is to make it impossible to create these "sort-of but
not really a ref" values.
The idea is simple in theory but gives rise to quite a large patch. Main
changes are:
-
MWasmDerivedPointer::New, MWasmDerivedIndexPointer::New: assert the base
type to exclude reftypes, and add comments. -
change from a single
address
to abase + constant offset
formulation, in:- wasm::EmitWasmPreBarrierGuard
- wasm::EmitWasmPreBarrierCall
In places where we have a singleaddress
but know it is "safe", because
either it's non-reftyped, or isn't live across any potential GC event, but
we need to use abase + constant offset
formulation, zero is passed for
the offset.
-
add a
base + constant offset
formulation for the following, but leave the
existingaddress
-only formalation in place:- Instance::postBarrierPreciseWithOffset
-
remove FunctionCompiler::writeGcValueAtAddress and route all such traffic
through FunctionCompiler::writeGcValueAtBasePlusOffset -
For constructors of MWasmStoreRef, MWasmLoadField, MWasmLoadFieldKA,
MWasmStoreFieldKA, MWasmStoreFieldRefKA, accept an offset of type size_t,
but restrict it to 0 .. 2^31-1 (a pre-existing limitation from
MWasmDerivedPointer does) so as to ensure that it is a valid offset to give
to a macroassemblerAddress
constructor. However, actually store such
values in the MIR as a uint32_t so as not to waste space. -
[ridealong] add some missing
getExtras
(debug-printing) methods for
MWasmStoreRef MWasmLoadField MWasmLoadFieldKA MWasmStoreFieldKA
MWasmStoreFieldRefKA -
[ridealong] some minor rearranging of access method orderings for
MWasmLoadField et al to make them more consistent.
Pushed by jseward@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a8c356a58263 Disallow uses of MWasmDerived{,Index}Pointer with reftyped-bases. r=rhunt.
Comment 3•1 year ago
|
||
bugherder |
Description
•