The default bug view has changed. See this FAQ.

AlignedStorage/AlignedStorage2's default copy constructor violates strict aliasing

RESOLVED FIXED in Firefox 49

Status

()

Core
MFBT
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla49
Points:
---

Firefox Tracking Flags

(firefox48 wontfix, firefox49 fixed, firefox-esr45 wontfix)

Details

Attachments

(2 attachments)

(Assignee)

Description

11 months ago
Firefox (or SpiderMonkey at least) is crashy with GCC 6, see bug 1245783. I filed a GCC bug [0] and they did fix one issue on their end, but it didn't fix the SpiderMonkey crashes completely. The GCC devs pointed out our code for AlignedStorage/AlignedStorage2 might be wrong (they're not 100% certain yet).

AlignedStorage and AlignedStorage2 have a default copy constructor that will copy |Nbytes| from dest to source. Those stores might be reordered with loads that use |T*|.

IIUC the char* strict aliasing rule doesn't apply, because we're using an array and not a char* pointer.

The JS shell hits both the AlignedStorage and AlignedStorage2 issues. Giving AlignedStorage an explicit copy constructor that uses memcpy seems to fix part of the crashes (not sure if that's 100% correct) and I got rid of some AlignedStorage2 uses (bug 1269317) to fix the other failures, but obviously there are other consumers in the browser that might also be affected.

[0] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70526
(Assignee)

Comment 1

11 months ago
See also this comment:

https://dxr.mozilla.org/mozilla-central/rev/1461a4071341c282afcf7b72e33036412d2251d4/mfbt/Alignment.h#107-110

 * As an important side effect, pulling the storage into this template is
 * enough obfuscation to confuse gcc's strict-aliasing analysis into not giving
 * false negatives when we cast from the char buffer to whatever type we've
 * constructed using the bytes.

It seems the default copy constructor is not a 'false negative'...

I noticed Boost's aligned_storage is non-copyable, so they avoid this issue.
(Assignee)

Comment 2

11 months ago
(In reply to Jan de Mooij [:jandem] from comment #0)
> AlignedStorage and AlignedStorage2 have a default copy constructor that will
> copy |Nbytes| from dest to source.

Er, from source to dest, obviously.

Waldo, any thoughts on this?
Flags: needinfo?(jwalden+bmo)
Without keeping around a tag for the actual type of the current contents of the union, it looks like aliasing rules just hose us here, for any operation that requires knowing the type of the actual data.  I think this means we should probably remove AlignedStorage{,2} and replace every use with more well-typed structures, e.g. Variant in cases where a little extra size doesn't matter.  There seem to be around thirty uses -- not great, but certainly not impossible to work through and convert.
Flags: needinfo?(jwalden+bmo)
(Assignee)

Comment 4

11 months ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)
> I think this means we
> should probably remove AlignedStorage{,2} and replace every use with more
> well-typed structures, e.g. Variant in cases where a little extra size
> doesn't matter.

Variant is built on top of AlignedStorage though, AFAICS :) 

Well as a first step I can try to delete AlignedStorage{,2}'s copy constructors and see what happens.
(Assignee)

Updated

11 months ago
Flags: needinfo?(jdemooij)
(Assignee)

Comment 5

11 months ago
Created attachment 8751758 [details] [diff] [review]
Patch

This patch deletes the default copy constructor and assignment operators.

That broke 2 places, (1) RInstructionStorage in the JIT (I think the memcpy I added there is safe) and (2) js::HashTable.

HashMapEntry::swap used to swap the raw bytes instead of the actual T's. I changed this - it required move assignment operators in a few places (for mozilla::Swap).
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8751758 - Flags: review?(jwalden+bmo)
Comment on attachment 8751758 [details] [diff] [review]
Patch

Review of attachment 8751758 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/Snapshots.h
@@ +506,5 @@
>  
> +struct RInstructionStorage
> +{
> +    static const size_t Size = 4 * sizeof(uint32_t);
> +    mozilla::AlignedStorage<Size> mem;

Make this private?

::: js/src/vm/ObjectGroup.cpp
@@ +1371,5 @@
> +    void operator=(AllocationSiteKey&& key) {
> +        script = mozilla::Move(key.script);
> +        offset = key.offset;
> +        kind = key.kind;
> +        proto = mozilla::Move(key.proto);

Put this function directly beneath the move constructor function.  If you want all constructors adjacent, start with the copy constructor, then move constructor, then move assignment operator.

::: mfbt/Alignment.h
@@ +123,5 @@
> +
> +  AlignedStorage() = default;
> +
> +  // AlignedStorage is non-copyable: the default copy constructor violates
> +  // strict aliasing rules, see bug 1269319.

s/see/per/ to avoid a run-on sentence (and below).
Attachment #8751758 - Flags: review?(jwalden+bmo) → review+

Comment 7

10 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/82b8884c7ce3

Comment 8

10 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac5e8c0b9ec2

Comment 9

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/82b8884c7ce3
https://hg.mozilla.org/mozilla-central/rev/ac5e8c0b9ec2
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
(Assignee)

Comment 10

10 months ago
Created attachment 8758206 [details] [diff] [review]
Patch for Aurora

I'd like to uplift bug 1269317 and bug 1269319 to Aurora and ESR45, to fix crashes with GCC 6. The coming months more people and distros will update to newer GCC versions, so it will be great to have this fixed.

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: Lots of crashes with GCC 6 and newer.
[Describe test coverage new/current, TreeHerder]: Code is exercised by many tests.
[Risks and why]: Low risk. Patch has been on m-c for a week.
[String/UUID change made/needed]: None.
Attachment #8758206 - Flags: review+
Attachment #8758206 - Flags: approval-mozilla-esr45?
Attachment #8758206 - Flags: approval-mozilla-aurora?
status-firefox48: --- → affected
status-firefox-esr45: --- → affected
Comment on attachment 8758206 [details] [diff] [review]
Patch for Aurora

Taking it to simplify the life of developers and packagers
Attachment #8758206 - Flags: approval-mozilla-esr45?
Attachment #8758206 - Flags: approval-mozilla-esr45+
Attachment #8758206 - Flags: approval-mozilla-aurora?
Attachment #8758206 - Flags: approval-mozilla-aurora+

Comment 12

10 months ago
holding this uplift since it depend on bug 1269317 which has uplift conflicts currently
(Assignee)

Comment 13

10 months ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/b73cfa00dab3
status-firefox48: affected → fixed
(Assignee)

Comment 14

10 months ago
I backed this out from Aurora because it caused MSVC bustage (likely caused by VS 2013) and Android build errors (see below).

I don't see a simple way to fix the Windows bustage, so it's probably best to let this ride the trains. Also, yesterday jseward pointed out that Firefox doesn't work with GCC 6 anyway until we fix bug 1232696.

build/stlport/stlport/stl/_alloc.h:481:41: error: use of deleted function 'SpecialAllocator<AtForkFuncs>::SpecialAllocator(const SpecialAllocator<AtForkFuncs>&)'
build/src/mozglue/build/BionicGlue.cpp:35:8: error: use of deleted function 'mozilla::AlignedStorage2<T>::AlignedStorage2(const mozilla::AlignedStorage2<T>&) [with T = AtForkFuncs]'
status-firefox48: fixed → wontfix
status-firefox-esr45: affected → wontfix
You need to log in before you can comment on or make changes to this bug.