476345 - The memory allocator in nsCSSValue::Array is not alignment safe

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Martin Husemann]

The file layout/style/nsCSSValue.h overrides operator new for nsCSSValue::Array. This implementation does not work on alignement critical 64 bit architectures. The natural alignement requirements of nsCSSValue (8 byte, since it contains pointers in the union) do not fit the "(char*)this + sizeof(*this)" calculation which results in only 4 byte aligned pointers.

Comment 1

[Martin Husemann]

Attachment: Proof of concept hack to avoid the alignment issue

This is only a hack to verify that fixing this alignement avoids a crash I've been seeing when visiting some websites (facebook being an example). With the patch, everything works fine.

I bet NSPR offers better ALIGN/ROUNDUP or similar macros that preferably should be used for a real fix. I can test, or create a better fix if someone tells me which way it should be.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The far simpler fix would be to include the first item in the array in the Array struct; then the compiler would be required to include the alignment requirements of nsCSSValue as part of sizeof(nsCSSValue::Array), since sizeof() is (I believe) required to include the alignment requirements of the contents so that it can be used for allocating arrays.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Could you test this patch and check that it fixes the problem?

Comment 4

[Martin Husemann]

Nice solution - and empirically verified: works!

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 360007 [details] [diff] [review]
patch

I've never liked this approach in all the other places it's used in our code (since it depends on structure ordering, although maybe C++ guarantees that), but I guess now I know there's a reason for it.  (I should probably add it to the portability guidelines...)

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Er, sorry, meant to provide a new patch with missed substitution fixed.

And I'll attach a diff of the moved code separately.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: diff of moved code

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 360189 [details] [diff] [review]
patch

This looks fine, but can we NS_ABORT_IF_FALSE if someone passes 0 as the aItemCount to our operator new?

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 360189 [details] [diff] [review]
patch

Pretty straightforward portability fix that we should probably take on 1.9.1.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/mozilla-central/rev/d9eff1fb5e60

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Had to back out since it turns out that doesn't compile on Windows.
http://hg.mozilla.org/mozilla-central/rev/c5044341110e
http://hg.mozilla.org/mozilla-central/rev/a1f57294eb00

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

It turns out Windows doesn't like it if you forward-declare the same nested struct twice in a class declaration.  If I don't add the forward-decl (which duplicated an earlier one that I hadn't seen), it's fine.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/mozilla-central/rev/bded4e432e7a

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9c15e15e1361