Closed Bug 1810090 Opened 1 year ago Closed 1 year ago

Disallow uses of MWasmDerived{,Index}Pointer with reftyped-bases

Categories

(Core :: JavaScript: WebAssembly, defect)

defect

Tracking

()

RESOLVED FIXED
111 Branch
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.

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 a base + constant offset formulation, in:

    • wasm::EmitWasmPreBarrierGuard
    • wasm::EmitWasmPreBarrierCall
      In places where we have a single address 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 a base + constant offset formulation, zero is passed for
      the offset.
  • add a base + constant offset formulation for the following, but leave the
    existing address-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 macroassembler Address 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.
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 111 Branch
Regressions: 1811559
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: