Summary: | Misaligned memory access in CloneDeserializer on ARM (<v6) | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yong Li <yong.li.webkit> | ||||||||
Component: | WebCore Misc. | Assignee: | Yong Li <yong.li.webkit> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, barraclough, commit-queue, dave+webkit, deepak.m, eric, kimmo.t.kinnunen, loki, oliver, ossy, staikos, thomas, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Other | ||||||||||
OS: | Other | ||||||||||
Attachments: |
|
Description
Yong Li
2010-10-13 08:44:31 PDT
Created attachment 70612 [details]
the patch
trying to get a test case
Created attachment 70617 [details]
The patch (no test added because...)
no new test is added because the crash can be reproduced by loading some existing tests like: LayoutTests/fast/events/message-channel-gc-4.html
Comment on attachment 70617 [details] The patch (no test added because...) View in context: https://bugs.webkit.org/attachment.cgi?id=70617&action=review r- due to the issues I noted. The duplicate string copy is the biggest concern. > WebCore/bindings/js/SerializedScriptValue.cpp:824 > + if (reinterpret_cast<unsigned>(ptr) & (sizeof(T) - 1)) when casting a pointer to an integer type you should always use uintptr_t or intptr_t (in this case you want uintptr_t) > WebCore/bindings/js/SerializedScriptValue.cpp:828 > +#else Honestly I think that given the likelihood of an unaligned read we should probably just drop the alignment check on armv5 or lower and always do a memcpy > WebCore/bindings/js/SerializedScriptValue.cpp:924 > + // Use 32-character-long inline buffer as a fast path for small strings. > + Vector<UChar, 32> alignedBuffer(length); > + memcpy(alignedBuffer.data(), ptr, length * sizeof(UChar)); > + str = UString(alignedBuffer.data(), length); This results in multiple copies, rather than str = UString(....) you should do str = UString::adopt(alignedBuffer); (In reply to comment #3) > (From update of attachment 70617 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=70617&action=review > > r- due to the issues I noted. The duplicate string copy is the biggest concern. > > > WebCore/bindings/js/SerializedScriptValue.cpp:824 > > + if (reinterpret_cast<unsigned>(ptr) & (sizeof(T) - 1)) > > when casting a pointer to an integer type you should always use uintptr_t or intptr_t (in this case you want uintptr_t) > > > WebCore/bindings/js/SerializedScriptValue.cpp:828 > > +#else > > Honestly I think that given the likelihood of an unaligned read we should probably just drop the alignment check on armv5 or lower and always do a memcpy > > > WebCore/bindings/js/SerializedScriptValue.cpp:924 > > + // Use 32-character-long inline buffer as a fast path for small strings. > > + Vector<UChar, 32> alignedBuffer(length); > > + memcpy(alignedBuffer.data(), ptr, length * sizeof(UChar)); > > + str = UString(alignedBuffer.data(), length); > > This results in multiple copies, rather than str = UString(....) you should do > str = UString::adopt(alignedBuffer); Thanks a lot. I'll update the patch soon Created attachment 70621 [details]
updated
1. use uintptr_t to cast pointer to integer
2. use UString::adopt to avoid duplicate copy
3. remove the aligned code paths
Comment on attachment 70621 [details]
updated
Nice
Comment on attachment 70621 [details] updated Clearing flags on attachment: 70621 Committed r69682: <http://trac.webkit.org/changeset/69682> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/69682 might have broken GTK Linux 64-bit Debug Hmm, it looks like I missed that alignment difference when I fixed that warnings. I assumed the buffer of CloneDeserializer is allocated in a natural way, but the m_data of SerializedScriptValue is allocated as a char vector. Although the fix is good, you should consider the MIPS uses natural alignment as well. Well, MIPS has unaligned load/store instruction, but the GCC also warns on unaligned data access (see deepak's comments at bug 43963). My suggestion is to create a WTF_CPU_NATURAL_ALIGNMENT_IS_NEEDED macro in Platform.h, and set it for WTF_CPU_ARMV5_OR_LOWER and WTF_CPU_MIPS as well. So, we can use CPU(NATURAL_ALIGNMENT_IS_NEEDED) instead of current CPU(ARMV5_OR_LOWER) check. At first of all it would be nice someone who has MIPS to confirm this warning. (In reply to comment #10) > Hmm, it looks like I missed that alignment difference when I fixed that warnings. I assumed the buffer of CloneDeserializer is allocated in a natural way, but the m_data of SerializedScriptValue is allocated as a char vector. > Although the fix is good, you should consider the MIPS uses natural alignment as well. Well, MIPS has unaligned load/store instruction, but the GCC also warns on unaligned data access (see deepak's comments at bug 43963). > My suggestion is to create a WTF_CPU_NATURAL_ALIGNMENT_IS_NEEDED macro in Platform.h, and set it for WTF_CPU_ARMV5_OR_LOWER and WTF_CPU_MIPS as well. So, we can use CPU(NATURAL_ALIGNMENT_IS_NEEDED) instead of current CPU(ARMV5_OR_LOWER) check. > At first of all it would be nice someone who has MIPS to confirm this warning. CPU(NATURAL_ALIGNMENT_IS_NEEDED) is good idea. I used CPU(ARMV5_OR_LOWER) because I see it is also being used somewhere else for alignment isue. About the warning, probably we should find a better way to suppress it. reinterpret_cast_ptr is different from reinterpret_cast only for #if CPU(ARM) && COMPILER(GCC), and it asserts that the pointer is already aligned. ASSERT(isPointerTypeAlignmentOkay(reinterpret_cast<TypePtr>(ptr))); So even on ARMv6, the debug build will throw an assertion, which is also annoying. > CPU(NATURAL_ALIGNMENT_IS_NEEDED) is good idea. I used CPU(ARMV5_OR_LOWER) because I see it is also being used somewhere else for alignment isue. It may be that CPU(NATURAL_ALIGNMENT_IS_NEEDED) is not fine-grained enough. Long longs seem to cause crashes on ARMv7 too (at least on some platform+toolchain combination). Related bug: http://bugs.webkit.org/show_bug.cgi?id=48742 |