Bug 47594 – Misaligned memory access in CloneDeserializer on ARM (<v6)

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Yong Li]

Bug 45301 fixes the warning, but the problem still exists. On ARM (<v6), CloneDeserializer::readLittleEndian and readString can result misaligned memory access.

#if ASSUME_LITTLE_ENDIAN
template <typename T> static bool readLittleEndian(const uint8_t*& ptr, const uint8_t* end, T& value)
{
        if (ptr > end – sizeof(value))
            return false;

        if (sizeof(T) == 1)
            value = *ptr++;
        else {
            value = *reinterpret_cast_ptr<const T*>(ptr); // here I think we should do memcpy(&value, ptr, sizeof(value))
            ptr += sizeof(T);
        }
        return true;
    }

[Yong Li]

Created attachment 70612 [details]
the patch

trying to get a test case

[Yong Li]

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

[Oliver Hunt]

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);

[Yong Li]

(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

[Yong Li]

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

[Oliver Hunt]

Comment on attachment 70621 [details]
updated

Nice

[WebKit Commit Bot]

Comment on attachment 70621 [details]
updated

Clearing flags on attachment: 70621

Committed r69682: <http://trac.webkit.org/changeset/69682>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/69682 might have broken GTK Linux 64-bit Debug

[Gabor Loki]

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.

[Yong Li]

(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.

[Kimmo Kinnunen]

> 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