The compare function in MIMESniffing.cpp take a char* pointer, "data", and does: 274 if (info.flags & SkipWhiteSpace) { 275 size_t pos = 0; 276 skipWhiteSpace(data, pos, dataSize); 277 data += pos; 278 dataSize -= pos; 279 } So if data starts 4-byte aligned but starts with a single space, it will be moved 1 byte ahead and no longer be aligned. Then it calls "maskedCompare(info, data, info.size)", which does: const uint32_t* data32 = reinterpret_cast_ptr<const uint32_t*>(data); Which is invalid as data is not necessarily 4-byte aligned. In a debug build, reinterpret_cast_ptr will crash with an assertion failure: ASSERTION FAILED: isPointerTypeAlignmentOkay(reinterpret_cast<TypePtr>(ptr)) /home/jmason/dev/webkit/Source/WTF/wtf/StdLibExtras.h(101) : TypePtr reinterpret_cast_ptr(const void*) [with TypePtr = const unsigned int*] I think that the unaligned access is potentially serious here so it shouldn't just be covered up, but I have no idea how to fix it.
One way to fix this is by simply not using reinterpret_cast_ptr: - const uint32_t* data32 = reinterpret_cast_ptr<const uint32_t*>(data); + const uint32_t* data32 = static_cast<const uint32_t*>(static_cast<const void*>(data)); For ARMv7/BLACKBERRY this works fine, no warning and no ASSERT hit.
(In reply to comment #1) > One way to fix this is by simply not using reinterpret_cast_ptr: > > - const uint32_t* data32 = reinterpret_cast_ptr<const uint32_t*>(data); > + const uint32_t* data32 = static_cast<const uint32_t*>(static_cast<const void*>(data)); > > For ARMv7/BLACKBERRY this works fine, no warning and no ASSERT hit. Forgot to say, ARMv7 allows the possible unalignment, there is a speed penalty but the unaligned case seems to be rare.
Another way to deal with this is add the slow case, which has no problem with unalignment, since everything is char* based: diff --git a/Source/WebCore/platform/network/MIMESniffing.cpp b/Source/WebCore/platform/network/MIMESniffing.cpp index 5efd17f..f67adb5 100644 --- a/Source/WebCore/platform/network/MIMESniffing.cpp +++ b/Source/WebCore/platform/network/MIMESniffing.cpp @@ -233,11 +233,33 @@ static inline size_t dataSizeNeededForImageSniffing() return result; } +#if (CPU(ARM) || CPU(MIPS)) && COMPILER(GCC) +static inline bool maskedCompareSlowCase(const MagicNumbers& info, const char* data) +{ + const char* p = reinterpret_cast<const char*>(info.pattern); + const char* m = reinterpret_cast<const char*>(info.mask); + const char* d = reinterpret_cast<const char*>(data); + + size_t count = info.size; + + for (size_t i = 0; i < count; ++i) { + if ((*d++ & *m++) != *p++) + return false; + } + return true; +} +#endif + static inline bool maskedCompare(const MagicNumbers& info, const char* data, size_t dataSize) { if (dataSize < info.size) return false; +#if (CPU(ARM) || CPU(MIPS)) && COMPILER(GCC) + if (!isPointerTypeAlignmentOkay(data)) + return maskedCompareSlowCase(info, data); +#endif + const uint32_t* pattern32 = reinterpret_cast_ptr<const uint32_t*>(info.pattern); const uint32_t* mask32 = reinterpret_cast_ptr<const uint32_t*>(info.mask); const uint32_t* data32 = reinterpret_cast_ptr<const uint32_t*>(data);
(In reply to comment #3) > Another way to deal with this is add the slow case, which has no problem with unalignment, since everything is char* based: > > diff --git a/Source/WebCore/platform/network/MIMESniffing.cpp b/Source/WebCore/platform/network/MIMESniffing.cpp > index 5efd17f..f67adb5 100644 > --- a/Source/WebCore/platform/network/MIMESniffing.cpp > +++ b/Source/WebCore/platform/network/MIMESniffing.cpp > @@ -233,11 +233,33 @@ static inline size_t dataSizeNeededForImageSniffing() > return result; > } > > +#if (CPU(ARM) || CPU(MIPS)) && COMPILER(GCC) > +static inline bool maskedCompareSlowCase(const MagicNumbers& info, const char* data) > +{ > + const char* p = reinterpret_cast<const char*>(info.pattern); > + const char* m = reinterpret_cast<const char*>(info.mask); > + const char* d = reinterpret_cast<const char*>(data); > + > + size_t count = info.size; > + > + for (size_t i = 0; i < count; ++i) { > + if ((*d++ & *m++) != *p++) > + return false; > + } > + return true; > +} > +#endif > + > static inline bool maskedCompare(const MagicNumbers& info, const char* data, size_t dataSize) > { > if (dataSize < info.size) > return false; > > +#if (CPU(ARM) || CPU(MIPS)) && COMPILER(GCC) > + if (!isPointerTypeAlignmentOkay(data)) > + return maskedCompareSlowCase(info, data); > +#endif > + > const uint32_t* pattern32 = reinterpret_cast_ptr<const uint32_t*>(info.pattern); > const uint32_t* mask32 = reinterpret_cast_ptr<const uint32_t*>(info.mask); > const uint32_t* data32 = reinterpret_cast_ptr<const uint32_t*>(data); I prefer this solution to hacking the warning.
Created attachment 151798 [details] Patch
(In reply to comment #4) > I prefer this solution to hacking the warning. Yes, I much prefer the more explicit solution.
Where does the test "#if (CPU(ARM) || CPU(MIPS)) && COMPILER(GCC)" come from, though?
(In reply to comment #7) > Where does the test "#if (CPU(ARM) || CPU(MIPS)) && COMPILER(GCC)" come from, though? It is the guard for isPointerTypeAlignmentOkay.
(In reply to comment #8) > (In reply to comment #7) > > Where does the test "#if (CPU(ARM) || CPU(MIPS)) && COMPILER(GCC)" come from, though? > > It is the guard for isPointerTypeAlignmentOkay. I wonder if we should move it inside isPointerTypeAlignmentOkay: #if (CPU(ARM) || CPU(MIPS)) && COMPILER(GCC) ... existing code #else return true; #endif To avoid having to repeat it whenever we call isPointerTypeAlignmentOkay.
(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > > > Where does the test "#if (CPU(ARM) || CPU(MIPS)) && COMPILER(GCC)" come from, though? > > > > It is the guard for isPointerTypeAlignmentOkay. > > I wonder if we should move it inside isPointerTypeAlignmentOkay: > > #if (CPU(ARM) || CPU(MIPS)) && COMPILER(GCC) > ... existing code > #else > return true; > #endif > > To avoid having to repeat it whenever we call isPointerTypeAlignmentOkay. That is a good idea. However, possibly this method was introduced as a quick "internal" check method. The background of this code is not clear to me just from looking at the commit message though.
(In reply to comment #10) > That is a good idea. However, possibly this method was introduced as a quick "internal" check method. The background of this code is not clear to me just from looking at the commit message though. That's my thought - if it was meant to be internal to reinterpret_cast_ptr, and we're now expanding it just to decide whether to call reinterpret_cast_ptr or use a slow path, we'd want to always use the same guard. But if it's meant to be a more general alignment check function, presumably it's usable on other platforms in different situations. Assuming the former, how about renaming it to "isPointerAlignmentOkayForReinterpretCast" to make it clear that it's not for general use?
Created attachment 151982 [details] Patch
Created attachment 151989 [details] Patch
Created attachment 152768 [details] Patch
Comment on attachment 152768 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152768&action=review > Source/WTF/ChangeLog:3 > + alignment crash in MIMESniffer alignment => Alignment? > Source/WTF/wtf/StdLibExtras.h:94 > -#if (CPU(ARM) || CPU(MIPS)) && COMPILER(GCC) > template<typename Type> > bool isPointerTypeAlignmentOkay(Type* ptr) > { > +#if (CPU(ARM) || CPU(MIPS)) && COMPILER(GCC) > return !(reinterpret_cast<intptr_t>(ptr) % __alignof__(Type)); > +#else > + return true; > +#endif > } > > +#if (CPU(ARM) || CPU(MIPS)) && COMPILER(GCC) I would define another isPointerTypeAlignmentOkay in the other block like reinterpret_cast_ptr()
Created attachment 152850 [details] Patch
(In reply to comment #16) > Created an attachment (id=152850) [details] > Patch This latest patch is more correct, I noticed it does need the reinterpret_cast before doing isPointerTypeAlignmentOkay. Unfortunately this re-adds the warning on gcc that goes with reinterpret_cast. The other thing is the test, for some reason this is still hard to create. It seems the mimetype should be unknown to trigger the problem...
Created attachment 153015 [details] Patch
Created attachment 153035 [details] Patch
Landed in r122990.