1879096 - Invalid `br_on_null` instruction validates

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, defect, P1)

Description

[fitzgen]

Attachment: unreachable-and-cast-v2.wasm

Steps to reproduce:

Validate the following Wasm:

(module
  (func (param anyref) (result anyref)
    ref.null struct
    local.get 0
    br_on_null 0
    drop
    br_on_cast_fail 0  structref structref
  )
)

(I've also attached the assembled .wasm binary of this WAT)

Actual results:

SpiderMonkey reports it as valid.

Expected results:

It should have failed with

type mismatch: expected structref, found anyref

at the br_on_cast_fail instruction, although the root cause of the issue is the br_on_null. The br_on_null should be typed [anyref anyref] -> [anyref (ref any)] due to the label type. The validator's stack going into the instruction is [structref anyref] which matches the isntruction due to structref <: anyref, but it seems that SpiderMonkey is not handling the result correctly. The instruction should effectively erase some type information about the structref, turning it into an anyref, but SpiderMonkey doesn't lose that information.

The validator should be doing:

pop_operands(label_types)
push_operands(label_types)

But I suspect it is either doing

push_operands(pop_operands(label_types))

or simply leaving the label types on the stack without any pushing/popping and only checking that the types on top of the stack match the label types.

Comment 1

[fitzgen]

See also https://github.com/WebAssembly/gc/issues/516

It turns out that wasm-smith has this essentially this same bug when generating "valid" Wasm modules.

Comment 2

[Ryan Hunt [:rhunt]]

I saw this upstream in the GC repo, thanks for filing this and summarizing it well! I believe we may also have a similar issue in br_on_cast instructions. Gonna need to investigate this a bit.

Comment 3

[Ryan Hunt [:rhunt]]

As the bug seems to be that we're preserving accurate type information when wasm is dictating that we should be erasing it, I don't believe this can lead to miscompilations. Setting severity appropriately. We should fix this quick though.

Comment 4

[Ben Visness [:bvisness]]

Attachment: Bug 1879096: Rewrite stack types in all conditional branch instructions. r=rhunt

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c3ec3cb9cbcb
Rewrite stack types in all conditional branch instructions. r=rhunt

Comment 6

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/c3ec3cb9cbcb