Copying RInstructionStorage by memcpy violates strict aliasing and induces undefined behavior

RESOLVED FIXED in Firefox 54
(NeedInfo from)

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: Waldo, Assigned: Waldo, NeedInfo)

Tracking

(Blocks: 1 bug)

unspecified
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(3 attachments)

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
Created attachment 8840234 [details] [diff] [review]
Remove mozilla::AlignedStorage, and inline its sole use into js::jit::RInstructionStorage

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)
Blocks: 1338374
Keywords: leave-open
Created attachment 8840267 [details] [diff] [review]
Use alignas/alignof (rather than a union that attempts to replicate the same thing) inside RInstructionStorage

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 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+
Attachment #8840267 - Flags: review?(nicolas.b.pierron) → review+
Created attachment 8840483 [details] [diff] [review]
Replace RInstructionStorage copy by a cloneInto function on every RInstruction.
Attachment #8840483 - Flags: review?(jwalden+bmo)

Comment 5

3 months ago
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
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+
https://hg.mozilla.org/mozilla-central/rev/7e72aa400561
https://hg.mozilla.org/mozilla-central/rev/039475e39939
Keywords: leave-open

Updated

3 months ago
Duplicate of this bug: 1343214
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

3 months 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
https://hg.mozilla.org/mozilla-central/rev/06b3c4468a68
https://hg.mozilla.org/mozilla-central/rev/109aaf987508
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.