[webkit-reviews] review denied: [Bug 47594] Misaligned memory access in CloneDeserializer on ARM (<v6) : [Attachment 70617] The patch (no test added because...)

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver at apple.com> has denied Yong Li <yong.li.webkit at gmail.com>'s
request for review:
Bug 47594: Misaligned memory access in CloneDeserializer on ARM (<v6)
https://bugs.webkit.org/show_bug.cgi?id=47594

Attachment 70617: The patch (no test added because...)
https://bugs.webkit.org/attachment.cgi?id=70617&action=review

------- Additional Comments from Oliver Hunt <oliver at apple.com>
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);