1682584 - Assert pointer is aligned to Value size in Poison functions

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P5)

Description

[Tooru Fujisawa [:arai]]

bug 1677224 comment #7 failure is caused by passing non-Value-size-aligned pointer to AlwaysPoison.
the function signature or comment doesn't imply the pointer should be Value-size-aligned,
but the underlying impl casts the pointer to JS::Value*.

It should have debug assert about the alignment

Comment 1

[Tooru Fujisawa [:arai]]

if we pass not-Value-size-aligned pointer to Poison function, it results in Segmentation Fault in PodSet.

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Bug 1682584 - Assert pointer is aliged to JS::Value size in Poison functions on 64-bit arch. r?jonco!

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1e7cd648ecd5
Assert pointer is aliged to JS::Value size in Poison functions. r=jonco

Comment 4

[Razvan Maries]

Backed out for perma failures.

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&searchStr=linux%2Cdebug%2Cspidermonkey%2Cbuilds%2Cspidermonkey-sm-arm-sim-linux32%2Fdebug%2Carm&revision=1e7cd648ecd59c6757ecaaa2d6d7b90d285da603&selectedTaskRun=Sx5AB7v4RRGaX8UeXg9CRg.1

Logs:
https://treeherder.mozilla.org/logviewer?job_id=324798682&repo=autoland&lineNumber=3230
https://treeherder.mozilla.org/logviewer?job_id=324799862&repo=autoland&lineNumber=1704

Backout: https://hg.mozilla.org/integration/autoland/rev/fe225435be7c3417e5a572d740e5695784e6b6ff

Comment 5

[Tooru Fujisawa [:arai]]

looks like the assumption here holds only on 64-bit arch.
on 32-bit arch, minimum requirement for the pointers passed to Poison are 4-bytes-aligned (=pointer-size), and that doesn't match alignas(8) for JS::Value.
I wonder if reinterpret_cast<JS::Value*> is valid here.

also, if the pointer doesn't align with 8 bytes, filling the memory block with poisoned object from the beginning doesn't contribute to safe crash,
given that value tag isn't in correct position when reading the block as Value.

maybe we should treat leading non-Value-size-aligned block separately on 32-bit arch?

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a092c8578144
Assert pointer is aliged to JS::Value size in Poison functions on 64-bit arch. r=jonco

Comment 7

[Cosmin Sabou [:CosminS]]

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