Reject out-of-range NaN payloads and bare type-index value types#2563
Conversation
A NaN literal's payload `n` must satisfy `1 <= n < 2^signif_bits` (52 bits for f64, 23 for f32). The parser masked the payload to the significand width instead of range-checking it, so an over-long payload such as `nan:0xffffffffffffff` (56 bits for f64) was silently truncated and accepted rather than rejected as the reference interpreter does. Range-check the payload against the significand width instead of masking. The spec testsuite only exercised the power-of-two boundary `nan:0x80_0000`, which the old masking rejected by coincidence (it masks to 0, hitting the "payload is zero => infinity" check), so out-of-range payloads that mask to a nonzero value went unnoticed. Regression tests cover both the invalid cases and the adjacent valid boundaries.
715f590 to
4c40f51
Compare
alexcrichton
left a comment
There was a problem hiding this comment.
Thanks for this, and good catch! I've got some thoughts below on minor tweaks, but otherwise looks good to me 👍
| // The short form of a reference type (without a `0x63`/`0x64` | ||
| // prefix byte) is only an abbreviation for `ref null <ht>` where | ||
| // `<ht>` is an *abstract* heap type. A bare (non-negative) type | ||
| // index is not a value type, so reject concrete/exact heap types | ||
| // reached through this path even though they parse as heap types | ||
| // in their own right. | ||
| if let HeapType::Concrete(_) | HeapType::Exact(_) = hty { | ||
| return Err(crate::Error::new("malformed reference type", pos)); | ||
| } |
There was a problem hiding this comment.
Instead of catching the result of parsing here, could this directly parse AbstractHeapType above? That way there's no need to reclassify errors or recover from an otherwise-successful parse that should have failed. I think in that case this'll have to handle the 0x65 case for shared: true abstract heap types, but that's a pretty small amount of duplication
| ;; type 0 = (func), type 1 = (func (param <0x00>)) — bare index used as a value type. | ||
| (assert_malformed | ||
| (module binary | ||
| "\00asm\01\00\00\00\01\08\02\60\00\00\60\01\00\00") |
There was a problem hiding this comment.
would you be ok splitting this up like a number of other module binary tests throughout the test suite with annotations of what each byte or set of bytes are? For example annotations like this
4c40f51 to
b004290
Compare
The short form of a reference type (without a `0x63`/`0x64` prefix) is only an abbreviation for `ref null <abstract-heap-type>`. The decoder instead parsed a full heap type there, which additionally accepts a bare non-negative type index, so `0x00` was wrongly decoded as the value type `(ref null 0)`. Parse an abstract heap type directly in the short-form path (handling the `0x65` prefix for `shared` variants) so a bare index is rejected at decode time. This also covers the over-long-LEB variant (`ff 00`). In value-type position the error surfaces as `invalid value type`; in a reference-type-only position (e.g. a table element type) as `malformed reference type`. The legitimate `0x63 <idx>` / `0x64 <idx>` concrete reftypes and the `shared` abstract shorthands (`0x65 ...`) are unaffected.
b004290 to
37f2233
Compare
Fixes two cases where
wasm-toolsaccepts malformed input that the spec grammar and the reference interpreter (wasm -d) reject.Text parser silently masked over-long NaN payloads (
wast)A NaN literal's payload
nmust satisfy1 ≤ n < 2^signif_bits(52 bits for f64, 23 for f32). The parser masked the payload to the significand width instead of range-checking it, so an over-long payload was silently truncated and accepted. The spec testsuite asserts this (const.wast,float_literals.wast) but only with the valuenan:0x80_0000— which isexactly
2^23and masks to0, so the old buggy code rejected it by coincidence.Binary decoder accepted a bare type index as a value type (
wasmparser)The short form of a reference type (no
0x63/0x64prefix) is only an abbreviation forref null <abstract-heap-type>. The decoder instead read a full heap type there, so a bare non-negative type index was accepted as a value type: Now concrete/exact heap types reached through the short-form path are rejected withmalformed reference typeat decode time. This also covers the over-long LEB variant (ff 00).