Bug 48742 – Misaligned memory access in CloneDeserializer on all ARM arch.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Kimmo Kinnunen]

http://bugs.webkit.org/show_bug.cgi?id=47594

There's a problem on our toolchain:
sbox-arm-none-linux-gnueabi-g++ (4.4.1)
WTF_ARM_ARCH_VERSION = 7

events/message-channel-gc-4.html --> pass

Window/window-postmessage-clone.html --> bus error crash

--

(gdb) bt
#0  0x3bd2fef8 in bool WebCore::CloneDeserializer::readLittleEndian<unsigned long long>(unsigned char const*&, unsigned char const*, unsigned long long&) ()
   from ./libQtWebKit.so.4.9.0
#1  0x3bd2cc40 in bool WebCore::CloneDeserializer::readLittleEndian<unsigned long long>(unsigned long long&) () from ./libQtWebKit.so.4.9.0
#2  0x3bd28280 in WebCore::CloneDeserializer::read(double&) ()
   from ./libQtWebKit.so.4.9.0
#3  0x3bd28cf0 in WebCore::CloneDeserializer::readTerminal() ()
   from ./libQtWebKit.so.4.9.0
#4  0x3bd29ff0 in WebCore::CloneDeserializer::deserialize() ()
   from ./libQtWebKit.so.4.9.0
#5  0x3bd27e1c in WebCore::CloneDeserializer::deserialize(JSC::ExecState*, JSC::JSGlobalObject*, WTF::Vector<unsigned char, 0u> const&) ()
   from ./libQtWebKit.so.4.9.0
#6  0x3bd2aad0 in WebCore::SerializedScriptValue::deserialize(JSC::ExecState*, JSC::JSGlobalObject*) () from ./libQtWebKit.so.4.9.0
#7  0x3ba4fd34 in WebCore::jsMessageEventData(JSC::ExecState*, JSC::JSValue, JSC::Identifier const&) () from ./libQtWebKit.so.4.9.0
#8  0x3c95aee8 in cti_op_get_by_id_custom_stub () from ./libQtWebKit.so.4.9.0
#9  0x3c95aee8 in cti_op_get_by_id_custom_stub () from ./libQtWebKit.so.4.9.0

[Gabor Loki]

> There's a problem on our toolchain:
> sbox-arm-none-linux-gnueabi-g++ (4.4.1)
> WTF_ARM_ARCH_VERSION = 7

Alignment problem on ARMv7? It is strange.

> Window/window-postmessage-clone.html --> bus error crash

This test should be on the skipped list for Qt (missing eventSender.beginDragWithFiles).

See the output of this test on ARMv7:
@@ -1,3 +1,5 @@
+CONSOLE MESSAGE: line 163: TypeError: Result of expression 'eventSender.beginDragWithFiles' [undefined] is not a function.
+FAIL: Timed out waiting for notifyDone to be called
 Tests that we clone object hierarchies
 PASS: 'postMessage(cyclicObject)' threw TypeError: Cannot post cyclic structures.
 PASS: 'postMessage(cyclicArray)' threw TypeError: Cannot post cyclic structures.
@@ -26,9 +28,4 @@
 PASS: eventData is 2009-02-13T23:31:30.000Z of type object
 PASS: eventData is [object Object] of type object
 PASS: eventData is [object Array](default toString threw RangeError: Maximum call stack size exceeded.) of type object
-PASS: eventData is [object File] of type object
-PASS: eventData is [object FileList] of type object
-PASS: eventData is [object ImageData] of type object
-PASS: eventData is [object ImageData] of type object
-PASS: eventData is done of type string

[Kimmo Kinnunen]

(In reply to comment #1)
> > There's a problem on our toolchain:
> > sbox-arm-none-linux-gnueabi-g++ (4.4.1)
> > WTF_ARM_ARCH_VERSION = 7
> 
> Alignment problem on ARMv7? It is strange.
> 
> > Window/window-postmessage-clone.html --> bus error crash
> 
> This test should be on the skipped list for Qt (missing eventSender.beginDragWithFiles).

The crash occurs with the actual when opening the file with a browser.

[Kimmo Kinnunen]

> > > Window/window-postmessage-clone.html --> bus error crash
> > 
> > This test should be on the skipped list for Qt (missing eventSender.beginDragWithFiles).
> 
> The crash occurs with the actual when opening the file with a browser.

Apparently ARMv7 shouldn't be able to do multi-word unaligned reads. If it does, your kernel probably does the fixups and restores the control. This is probably as slow as it sounds (kernel trap) and should be fixed in userland code. 

You can cat /proc/cpu/alignment to see the fixups.

http://wiki.debian.org/ArmEabiFixes

[Gabor Loki]

> Apparently ARMv7 shouldn't be able to do multi-word unaligned reads. If it does, your kernel probably does the fixups and restores the control. This is probably as slow as it sounds (kernel trap) and should be fixed in userland code. 

That is true. I have just checked the disassembled code, and see a LDRD instruction which tries to access an odd address. This is definitely an alignment fault on every ARM arch.
Well, I will design a better construct (probably a template) to handle this situation also.

Btw, I think that using kernel trap to fix unaligned data access is better than a crash. ;)

[Gabor Loki]

Created attachment 72929 [details]
Misaligned memory access in CloneDeserializer on all ARM arch.

This will fix the missaligned memory access with GCC and RVCT on ARM. The MSCV fix is speculative.

[WebKit Review Bot]

Attachment 72929 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'JavaScriptCore/ChangeLog', u'JavaScriptCore/wtf/StdLibExtras.h', u'WebCore/ChangeLog', u'WebCore/bindings/js/SerializedScriptValue.cpp']" exit_code: 1
JavaScriptCore/wtf/StdLibExtras.h:94:  unaligned_cast_by_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/StdLibExtras.h:103:  unaligned_cast_by_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/StdLibExtras.h:129:  unaligned_cast_by_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/StdLibExtras.h:138:  unaligned_cast_by_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/StdLibExtras.h:156:  unaligned_cast_by_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/StdLibExtras.h:162:  unaligned_cast_by_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 6 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gabor Loki]

Created attachment 73345 [details]
Misaligned memory access in CloneDeserializer on all ARM arch.

Fix readTime at WebCore/plugins/PluginDatabase.cpp.

[WebKit Review Bot]

Attachment 73345 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'JavaScriptCore/ChangeLog', u'JavaScriptCore/wtf/StdLibExtras.h', u'WebCore/ChangeLog', u'WebCore/bindings/js/SerializedScriptValue.cpp', u'WebCore/plugins/PluginDatabase.cpp']" exit_code: 1
JavaScriptCore/wtf/StdLibExtras.h:94:  unaligned_cast_by_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/StdLibExtras.h:103:  unaligned_cast_by_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/StdLibExtras.h:129:  unaligned_cast_by_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/StdLibExtras.h:138:  unaligned_cast_by_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/StdLibExtras.h:156:  unaligned_cast_by_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/StdLibExtras.h:162:  unaligned_cast_by_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 6 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gavin Barraclough]

Just a thought, but it would probably be more optimal on all platforms to ensure that 16-bit data is naturally aligned.

A one byte pad on unaligned strings is not likely to significantly increase the size of our serialized format.  I don't really know this code, but looking through it looks like we could probably add a NOP pad in write(const Idendentifier&) adding this to force alignment conditionally as needed.

Then, in readString, when we cast the pointer to a UChar* we could also assert that it is UChar aligned.   Cleaning up the serialization in this fashion would seem like a better solution for ARM – better to actually make the data naturally aligned than work around it not being so.

[Oliver Hunt]

(In reply to comment #9)
> Just a thought, but it would probably be more optimal on all platforms to ensure that 16-bit data is naturally aligned.
> 
> A one byte pad on unaligned strings is not likely to significantly increase the size of our serialized format.  I don't really know this code, but looking through it looks like we could probably add a NOP pad in write(const Idendentifier&) adding this to force alignment conditionally as needed.
> 
> Then, in readString, when we cast the pointer to a UChar* we could also assert that it is UChar aligned.   Cleaning up the serialization in this fashion would seem like a better solution for ARM – better to actually make the data naturally aligned than work around it not being so.

Strings don't need padding as they're using 16bit characters, it is various tokens that are only one byte in size, if we think it's worth bumping all symbols that aren't multiple of 2 bytes up to two byte boundaries we should do that as soon as possible (eg. before localstorage, etc start using this)

[Gabor Loki]

(In reply to comment #10)
> if we think it's worth bumping all symbols that aren't multiple of 2 bytes up to two byte boundaries we should do that as soon as possible (eg. before localstorage, etc start using this)

Well, it looks better, but 'long long' data should be aligned to word boundary (see LDRD instruction on ARM).

[Oliver Hunt]

I think the safest solution to this problem is simply to make the clone serializer and deserialiser use the byte at a time copy methods for all archs that require alignment, in addition to being the incorrect endianess

[Gabor Loki]

(In reply to comment #12)
Oliver, if I understand your last comment correctly, you won't change the structure of the container. You would like those arch. which needs aligned addressing to read/write the unaligned data as a byte array. If this is what you want, please review the patch. My patch reads/writes unaligned data as a byte array.

Btw, addressing with a packed-structure is much faster than with memcpy if the length of the data is small (for example: an unaligned long long data). If the addressed data is long (like a string), the memcpy is much faster than the packed-structure.

[Yong Li]

936  Vector<UChar> alignedBuffer(length);
937  memcpy(alignedBuffer.data(), ptr, length * sizeof(UChar));
938  str = UString::adopt(alignedBuffer);
 931 if (isPointerTypeAlignmentOkay(reinterpret_cast<const UChar*>(ptr)))
 932 str = UString(reinterpret_cast<const UChar*>(ptr), length);
 933 else {
 934 Vector<UChar> alignedBuffer(length);
 935 memcpy(alignedBuffer.data(), ptr, length * sizeof(UChar));
 936 str = UString::adopt(alignedBuffer);
 937 }
939938 #else 

 why is this change necessary? Anyway, we need to allocate memory for the string. will the new code path be faster?

[Oliver Hunt]

I think the ARM guards here are a bad choice, as there are non-ARM platforms that expect aligned reads (MIPS i think).  Honestly I think the various ARM guards make the code unpleasant to read and think a better approach is simply to make all platforms with alignment constraints use the generic byte at a time model used for non-little endian platforms.

[Oliver Hunt]

I believe the simplest solution to this problem is to make all architectures that have alignment requirements just take the bigendian path.  That will result in a single localised change and won't result in ifdef's being strewn around everywhere.

[David Kilzer (:ddkilzer)]

Comment on attachment 73345 [details]
Misaligned memory access in CloneDeserializer on all ARM arch.

r- based on Comment #15 and Comment #16.

[Oliver Hunt]

Created attachment 84330 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 84330 [details]
Patch

r+, thanks for fixing this for ARM (having tested it).

[Oliver Hunt]

Committed r80070: <http://trac.webkit.org/changeset/80070>