Closed
Bug 1341951
Opened 8 years ago
Closed 8 years ago
Copying RInstructionStorage by memcpy violates strict aliasing and induces undefined behavior
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: Waldo, Assigned: Waldo)
References
Details
Attachments
(3 files)
5.12 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
4.89 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
23.15 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
Recover code currently assumes it can copy around RInstructionStorage -- which is basically a bag'o'bytes into which various RInstruction subclasses are periodically written -- by memcpy.
This is wrong.
It's safe to copy the bytes of one T, into another T. But it isn't safe to copy the bytes of a T, *not* into a previously-constructed T, and then reinterpret that non-T location as a T. And RInstructionStorage's copy constructor and assignment operator do exactly this.
We've hit crashes before with this tactic. See bug 1269319. So this needs to be fixed, ideally pretty quickly because any failures this causes will be noise
Assignee | ||
Comment 1•8 years ago
|
||
This isn't a fix at all. But it does get rid of mfbt's highly-dangerous footgun by putting this Bad Implementation Tactic in the single remaining place that does it. And that's a very good thing.
Once that's done, fixing the recover code can become your problem. ;-) I looked at this problem enough to conclude that I couldn't easily fix it without learning a bunch about recover code first, which I do not have time to do. Unfortunately, this requires a subject-matter expert to fix it.
Attachment #8840234 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Updated•8 years ago
|
Blocks: 1338374
Keywords: leave-open
Assignee | ||
Comment 2•8 years ago
|
||
Still not a fix, but I had this patch further down in my mq, and I might as well get it out.
Attachment #8840267 -
Flags: review?(nicolas.b.pierron)
Comment 3•8 years ago
|
||
Comment on attachment 8840234 [details] [diff] [review]
Remove mozilla::AlignedStorage, and inline its sole use into js::jit::RInstructionStorage
Review of attachment 8840234 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jit/Snapshots.h
@@ +506,5 @@
>
> class RInstructionStorage
> {
> + /*
> + * DO NOT REUSE THIS IMPLEMENTATION TACTIC.
nit: usually we use // comments in the Jit, but feel free to land it as-is as I will remove it in an upcoming patch to this bug.
Attachment #8840234 -
Flags: review?(nicolas.b.pierron) → review+
Updated•8 years ago
|
Attachment #8840267 -
Flags: review?(nicolas.b.pierron) → review+
Comment 4•8 years ago
|
||
Attachment #8840483 -
Flags: review?(jwalden+bmo)
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e72aa400561
Remove mozilla::AlignedStorage, and inline its sole use into js::jit::RInstructionStorage. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/039475e39939
Use alignas/alignof (rather than a union that attempts to replicate the same thing) inside RInstructionStorage. r=nbp
Assignee | ||
Comment 6•8 years ago
|
||
Comment on attachment 8840483 [details] [diff] [review]
Replace RInstructionStorage copy by a cloneInto function on every RInstruction.
Review of attachment 8840483 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jit/Recover.h
@@ +193,5 @@
>
> uint32_t pcOffset() const {
> return pcOffset_;
> }
> + uint32_t numOperands() const override {
All these changes from here on down in this file are unrelated to this bug/patch, right? Would be nice to split them into their own rev.
::: js/src/jit/Snapshots.cpp
@@ +602,5 @@
> +RecoverReader::RecoverReader(const RecoverReader& rr)
> + : reader_(rr.reader_),
> + numInstructions_(rr.numInstructions_),
> + numInstructionsRead_(rr.numInstructionsRead_)
> +{
resumeAfter_ doesn't need to be copied here, and assigned below? (...why do I have this feeling I've seen this before?)
::: js/src/jit/Snapshots.h
@@ +522,5 @@
>
> RInstructionStorage() = default;
>
> + // Making a copy of raw bytes holding a RInstruction instance would be a
> + // strict aliasing violations: see the comment above, and see bug 1269319
s/violations/violation/, and remove the "see the comment above" bit.
@@ +554,5 @@
> void readInstruction();
>
> public:
> RecoverReader(SnapshotReader& snapshot, const uint8_t* recovers, uint32_t size);
> + RecoverReader(const RecoverReader& rr);
This probably needs an explicit on it for it to be landable.
Attachment #8840483 -
Flags: review?(jwalden+bmo) → review+
Comment 7•8 years ago
|
||
bugherder |
Assignee | ||
Updated•8 years ago
|
Keywords: leave-open
Nicolas, do you mind landing the follow-up patch? I seem to find some issues that may be related to Waldo's alignas/alignof use patch, that Waldo says might be fixed by your follow-up patch.
Flags: needinfo?(nicolas.b.pierron)
Actually, the patch does fix the issue, which seems to be compiler-specific (6.2.0, only on 1 of my machines) for the following testcase:
function f() {
try {
e;
} catch (e) {}
}
f("");
f("z");
Tested with m-c rev 1bc2ad020aee, run with --fuzzing-safe --no-threads --ion-eager
Comment 11•8 years ago
|
||
Pushed by npierron@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/06b3c4468a68
Replace RInstructionStorage copy by a cloneInto function on every RInstruction. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/109aaf987508
Use override keyword for all virtual methods of Recover Instructions. r=Waldo
Comment 12•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/06b3c4468a68
https://hg.mozilla.org/mozilla-central/rev/109aaf987508
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 13•7 years ago
|
||
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #9)
> Nicolas, do you mind landing the follow-up patch?
Done. (4 months ago)
Flags: needinfo?(nicolas.b.pierron)
You need to log in
before you can comment on or make changes to this bug.
Description
•