Last Comment Bug 715074 - SIGBUS on unaligned access in Key::EncodeNumber
: SIGBUS on unaligned access in Key::EncodeNumber
Status: RESOLVED FIXED
: crash, regression
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: ARM All
: -- normal (vote)
: ---
Assigned To: Jan Varga [:janv]
:
Mentors:
: 715043 (view as bug list)
Depends on: 692614
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-03 23:34 PST by Michael Wu [:mwu]
Modified: 2012-02-09 16:53 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
unaffected
+
fixed
fixed


Attachments
patch v0.1 (2.43 KB, patch)
2012-01-04 10:14 PST, Jan Varga [:janv]
no flags Details | Diff | Splinter Review
patch v0.2 (1.83 KB, patch)
2012-01-05 07:14 PST, Jan Varga [:janv]
jonas: review+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Michael Wu [:mwu] 2012-01-03 23:34:33 PST
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 [:khuey] (khuey@mozilla.com) 2012-01-04 04:31:31 PST
Jan, can you take this?
Comment 3 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-01-04 07:21:38 PST
*** Bug 715043 has been marked as a duplicate of this bug. ***
Comment 4 Jan Varga [:janv] 2012-01-04 10:14:27 PST
Created attachment 585802 [details] [diff] [review]
patch v0.1

mwu, could you please try this patch ?
Comment 5 Michael Wu [:mwu] 2012-01-04 10:48:01 PST
(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) PTO Until July 5th 2012-01-04 10:57:58 PST
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] 2012-01-04 11:16:18 PST
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) PTO Until July 5th 2012-01-04 11:46:21 PST
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] 2012-01-04 12:04:17 PST
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] 2012-01-04 12:10:08 PST
And aPos would have to be incremented appropriately
Comment 11 Jan Varga [:janv] 2012-01-04 12:14:39 PST
(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] 2012-01-04 12:23:47 PST
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] 2012-01-04 12:25:38 PST
Hmm, the current implementation should be faster, maybe twice, no ?
Comment 14 Michael Wu [:mwu] 2012-01-04 12:52:48 PST
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] 2012-01-04 13:04:29 PST
ok, let's use memcpy
Comment 16 Jonas Sicking (:sicking) PTO Until July 5th 2012-01-04 13:41:20 PST
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 2012-01-05 01:59:44 PST
NS_MIN<size_t>(a, b) should work as well
Comment 18 Jan Varga [:janv] 2012-01-05 02:15:46 PST
(In reply to Ms2ger from comment #17)
> NS_MIN<size_t>(a, b) should work as well

ok, thanks
Comment 19 Jan Varga [:janv] 2012-01-05 07:14:30 PST
Created attachment 586063 [details] [diff] [review]
patch v0.2
Comment 20 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-01-06 01:32:40 PST
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 2012-01-06 01:33:11 PST
(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) PTO Until July 5th 2012-01-06 01:46:08 PST
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) PTO Until July 5th 2012-01-06 01:49:51 PST
This should be landed on FF11 too.
Comment 24 Jan Varga [:janv] 2012-01-06 01:51:32 PST
(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] 2012-01-06 02:00:18 PST
landed on the trunk
https://hg.mozilla.org/mozilla-central/rev/8ae16e346bd0
Comment 26 Alex Keybl [:akeybl] 2012-02-06 15:34:15 PST
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] 2012-02-07 14:54:59 PST
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!) 2012-02-07 15:29:52 PST
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] 2012-02-09 13:33:31 PST
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!) 2012-02-09 16:53:46 PST
https://hg.mozilla.org/releases/mozilla-beta/rev/0534fda199cc

Note You need to log in before you can comment on or make changes to this bug.