1637682 - A few Metadata fields are not serialized and deserialized, thus they may be undefined

Status: RESOLVED FIXED

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

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks to dmajor who asked about this on slack.

A few fields in Metadata may not be serialized or deserialized: for instance, bigIntEnabled isn't serialized and deserialized here, leading to undefined behavior here.

Classifying as sec bug for now (I'm probably being paranoid), but I'd like that we take time thinking about what the consequences might be. v128enabled suffers from the same issue, as far as i can tell.

Comment 1

[Lars T Hansen [:lth]]

OK, good find. It's problematic that we don't have a system that catches these mistakes for us. We need to fix the problem for BigInt soonish because that rides the FF78 train.

Comment 2

[(Away)]

The clang-10 landing (bug 1616692) bounced because of a ubsan complaint about this (I'm not sure why clang-9's sanitizers don't catch it). I'm very eager to re-land clang-10, since compiler updates bitrot really fast and I'm always catching up with new test failures. I had been planning on adding a suppression for this error while it's investigated, but, seeing that this is potentially a security bug... is that an ok thing to do?

Comment 3

[Lars T Hansen [:lth]]

This is at most a nightly-only concern since bigint and simd are nightly-only at the moment. Simd is even off by default. As for bigint, it's a new feature no content uses, and it's a boolean flag, so if it should have been false but is read as true we're just enabling code that isn't exercised, and if it should have been true but is read as false then we're at most throwing errors some places we shouldn't be, and we'll see those bug reports. So suppressing it should be just fine.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

It's problematic that we don't have a system that catches these mistakes for us.

The right way is to put these raw fields in the MetadataCacheablePod, which are serialized and deserialized for free.

we're at most throwing errors some places we shouldn't be

Are we sure it might not cause MOZ_CRASHes? Even so, we could unhide this bug and suppress it then, because mozcrashes are safe.

Comment 5

[Lars T Hansen [:lth]]

Yeah, I can look into preparing a patch to move the fields tomorrow.

In general, the values of these switches that are exposed to about:config must be assumed to change at any moment, so if there's a MOZ_CRASH or other crash then that's a latent bug I think. I think we've generally been careful about this, we read switches once when compilation starts and then propagate those values, and we try to gate things at high levels (instructions that validate) but not low (specific code generation strategies), though of course bugs happen.

With that logic in mind, it sort of doesn't make a difference whether the fields are serialized, so long as we don't deserialize them but instead get current values...

Comment 6

[Lars T Hansen [:lth]]

omitsBoundsChecks falls into this category too. It was already properly serialized/deserialized but can be moved to the POD.

Comment 7

[Lars T Hansen [:lth]]

Attachment: Bug 1637682 - Make sure to serialize/deserialize all code properties. r=bbouvier

Serialized/deserialized properties that are POD should be in the
CacheablePod structure so that we don't have to worry about adding
extra code to serialize/deserialize them; it's so easy to forget.

Comment 8

[Lars T Hansen [:lth]]

Nightly-only and probably without any real user impact => sec-low

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

This has been backed out for failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=spidermonkey&revision=bc5e947dda0abeb48f46417f596a079fbc53817e

Comment 10

[Sylvestre Ledru [:Sylvestre]]

On Benjamin's request

Comment 11

[Lars T Hansen [:lth]]

Oops, forgot to update serializedSize() too.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/99ddf2c70b4f
Make sure to serialize/deserialize all code properties.  r=bbouvier

Comment 13

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/99ddf2c70b4f

Comment 14

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/db5f5d127e24
Backed out changeset bc5e947dda0a for wasm related bustage CLOSED TREE

Comment 15

[(Away)]

(In reply to Pulsebot from comment #14)

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/db5f5d127e24
Backed out changeset bc5e947dda0a for wasm related bustage CLOSED TREE

Note that this is a delayed reporting of the first backout; the second landing is still present.

Comment 16

[(Away)]

Thanks a lot for the quick response on this, Benjamin and Lars!