Closed Bug 1269319 Opened 8 years ago Closed 8 years ago

AlignedStorage/AlignedStorage2's default copy constructor violates strict aliasing

Categories

(Core :: MFBT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox48 --- wontfix
firefox49 --- fixed
firefox-esr45 --- wontfix

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(2 files)

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
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.
(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)
(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.
Flags: needinfo?(jdemooij)
Attached patch PatchSplinter Review
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+
https://hg.mozilla.org/mozilla-central/rev/82b8884c7ce3
https://hg.mozilla.org/mozilla-central/rev/ac5e8c0b9ec2
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Attached patch Patch for AuroraSplinter Review
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?
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+
holding this uplift since it depend on bug 1269317 which has uplift conflicts currently
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]'
You need to log in before you can comment on or make changes to this bug.