715074 - SIGBUS on unaligned access in Key::EncodeNumber

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Michael Wu [:mwu]]

While attempting to start the gallery app on b2g, we get:

Program received signal SIGBUS, Bus error.
0x8261568a in mozilla::dom::indexedDB::Key::EncodeNumber (
    this=<optimized out>, aFloat=<optimized out>, aType=<optimized out>)
    at /fasthome/mikew/moz/B2G/gecko/dom/indexedDB/Key.cpp:414
414     /fasthome/mikew/moz/B2G/gecko/dom/indexedDB/Key.cpp: No such file or directory.
        in /fasthome/mikew/moz/B2G/gecko/dom/indexedDB/Key.cpp
(gdb) bt
#0  0x8261568a in mozilla::dom::indexedDB::Key::EncodeNumber (
    this=<optimized out>, aFloat=<optimized out>, aType=<optimized out>)
    at /fasthome/mikew/moz/B2G/gecko/dom/indexedDB/Key.cpp:414
#1  0x82615aaa in mozilla::dom::indexedDB::Key::EncodeJSVal (this=0x4236c388, 
    aCx=0x402c1c40, aVal=..., aTypeOffset=<optimized out>)
    at /fasthome/mikew/moz/B2G/gecko/dom/indexedDB/Key.cpp:155
#2  0x8260455c in SetFromJSVal (aVal=<optimized out>, this=<optimized out>, 
    aCx=<optimized out>) at ../../dist/include/mozilla/dom/indexedDB/Key.h:166
#3  (anonymous namespace)::GetKeyFromJSVal (aCx=0x402c1c40, aVal=..., 
    aKey=..., aAllowUnset=<optimized out>)
    at /fasthome/mikew/moz/B2G/gecko/dom/indexedDB/IDBKeyRange.cpp:101
#4  0x82604578 in (anonymous namespace)::GetKeyFromJSValOrThrow (aCx=0x0, 
    aVal=..., aKey=...)
    at /fasthome/mikew/moz/B2G/gecko/dom/indexedDB/IDBKeyRange.cpp:132
#5  0x826047a6 in (anonymous namespace)::MakeLowerBoundKeyRange (
    aCx=0x402c1c40, aArgc=<optimized out>, aVp=0x40f00110)
    at /fasthome/mikew/moz/B2G/gecko/dom/indexedDB/IDBKeyRange.cpp:176
#6  0x82b7ff46 in CallJSNative (args=<optimized out>, native=<optimized out>, 
    cx=<optimized out>)
    at /fasthome/mikew/moz/B2G/gecko/js/src/jscntxtinlines.h:311

At that line - https://hg.mozilla.org/mozilla-central/file/a737cc816eec/dom/indexedDB/Key.cpp#l414 we see:

 *reinterpret_cast<PRUint64*>(buffer) = NS_SWAP64(number);

"buffer" isn't aligned enough.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Jan, can you take this?

Comment 2

[Jan Varga [:janv]]

http://root42.blogspot.com/2011/02/alignment-of-double-values-on-arm.html

Comment 4

[Jan Varga [:janv]]

Attachment: patch v0.1

mwu, could you please try this patch ?

Comment 5

[Michael Wu [:mwu]]

(In reply to Jan Varga [:janv] from comment #4)
> Created attachment 585802 [details] [diff] [review]
> patch v0.1
> 
> mwu, could you please try this patch ?

No crash now. The Makefile.in part didn't apply, though it doesn't matter for me.

Can we add padding to align the buffer instead?

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The whole point of this code is to create a compressed representation of the key as to reduce IO, so adding padding would defeat that.

Comment 7

[Michael Wu [:mwu]]

In that case, I would like to see the encoding match the decoding either by using for loops or memcpy.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

They can't be exactly the same since we do some post-processing after encoding. Please see last step of comment at the top of Key.cpp. But we could use memcpy on big-endian CPUs sure. But none of our tier-1 platforms are big-endian

Comment 9

[Michael Wu [:mwu]]

Well, it's not exactly the same, but it's close:

encode:
number = NS_SWAP64(number);
memcpy(buffer, &number, sizeof(number));

decode:
memcpy(&number, aPos, NS_MIN(sizeof(number), aEnd - aPos));
number = NS_SWAP64(number);

Comment 10

[Michael Wu [:mwu]]

And aPos would have to be incremented appropriately

Comment 11

[Jan Varga [:janv]]

(In reply to Michael Wu [:mwu] from comment #9)
> Well, it's not exactly the same, but it's close:
> 
> encode:
> number = NS_SWAP64(number);
> memcpy(buffer, &number, sizeof(number));
> 
> decode:
> memcpy(&number, aPos, NS_MIN(sizeof(number), aEnd - aPos));
> number = NS_SWAP64(number);


do you want this on all platforms ?

Comment 12

[Michael Wu [:mwu]]

Yes, since it should work on all platforms, though we should test first to make sure that gcc picks a memcpy implementation that handles unaligned addresses. It should since we're passing in char pointers, but I'll make sure on my device.

Comment 13

[Jan Varga [:janv]]

Hmm, the current implementation should be faster, maybe twice, no ?

Comment 14

[Michael Wu [:mwu]]

memcpy is generally optimized and will attempt to copy more than one byte at a time using hardware specific features if it is possible. So at the very least, decoding might be faster because we only increment aPos once and memcpy may copy more than one byte at a time. In the encoding case, the copy is always a fixed size, so the compiler may actually just replace memcpy with some simple copy instruction(s) on platforms where unaligned writes are supported.

But I really just want to do memcpy all the time to keep the code simple.

Comment 15

[Jan Varga [:janv]]

ok, let's use memcpy

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Actually, given that we're copying at the most 8 bytes here, I don't think there's any benefit to copying multiple bytes at a time. An unrolled loop is likely the fastest implementation.

However given that performance of the actual mem copy here is very unlikely to matter, I think we should use memcpy everywhere since it's the smallest amount of code, which is nice.

Note that it's fine to always do aPos += 8. It's ok for aPos to point past aEnd. We end up in that situation elsewhere already.

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

NS_MIN<size_t>(a, b) should work as well

Comment 18

[Jan Varga [:janv]]

(In reply to Ms2ger from comment #17)
> NS_MIN<size_t>(a, b) should work as well

ok, thanks

Comment 19

[Jan Varga [:janv]]

Attachment: patch v0.2

Comment 20

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

We're running IDB tests on fennec right?  Did we just happen to hit a funny case on b2g?  If so, can we get a test for that here?

Comment 21

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #20)
> Did we just happen to hit a funny case on b2g?

s/b2g/gaia/

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The basic indexedDB tests should be testing this quite extensively. Do we have those tests disabled on fennec by any chance?

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This should be landed on FF11 too.

Comment 24

[Jan Varga [:janv]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #20)
> We're running IDB tests on fennec right?  Did we just happen to hit a funny
> case on b2g?  If so, can we get a test for that here?

I don't think we're running these tests on fennec.

Comment 25

[Jan Varga [:janv]]

landed on the trunk
https://hg.mozilla.org/mozilla-central/rev/8ae16e346bd0

Comment 26

[Alex Keybl [:akeybl]]

Please nominate for Beta 11 approval ASAP if we'd like to get land prior to release, the user effect is significant, and the patch is considered low risk.

Comment 27

[Alex Keybl [:akeybl]]

Ben - can you fill out the form that is pre-populated in the comment when requesting beta approval? We'd like to better understand the risk and user benefit.

Comment 28

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 586063 [details] [diff] [review]
patch v0.2

Sorry, I misinterpreted your earlier comment (thought that you knew all this already since you were suggesting we nominate):

[Approval Request Comment]
Regression caused by (bug #): 692614
User impact if declined: IndexedDB crashes almost instantly on ARM devices, makes it impossible to use.
Testing completed (on m-c, etc.): Lots of m-c testing.
Risk to taking this patch (and alternatives if risky): Really low risk, we're using this code on desktop all the time and haven't had any problems.
String changes made by this patch: None

Comment 29

[Alex Keybl [:akeybl]]

Comment on attachment 586063 [details] [diff] [review]
patch v0.2

[Triage Comment]
Given the low risk nature of this patch, and where we are in the cycle, approving for Beta.

Comment 30

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://hg.mozilla.org/releases/mozilla-beta/rev/0534fda199cc