RESOLVED FIXED 89787
Alignment crash in MIMESniffer
https://bugs.webkit.org/show_bug.cgi?id=89787
Summary Alignment crash in MIMESniffer
Joe Mason
Reported 2012-06-22 14:53:13 PDT
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.
Attachments
Patch (1.65 KB, patch)
2012-07-11 15:20 PDT, Rob Buis
no flags
Patch (2.25 KB, patch)
2012-07-12 09:59 PDT, Rob Buis
no flags
Patch (2.21 KB, patch)
2012-07-12 10:27 PDT, Rob Buis
no flags
Patch (3.54 KB, patch)
2012-07-17 08:24 PDT, Rob Buis
no flags
Patch (3.54 KB, patch)
2012-07-17 15:31 PDT, Rob Buis
no flags
Patch (3.45 KB, patch)
2012-07-18 08:07 PDT, Rob Buis
no flags
Patch (3.43 KB, patch)
2012-07-18 10:18 PDT, Rob Buis
yong.li.webkit: review+
Rob Buis
Comment 1 2012-07-11 14:14:16 PDT
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.
Rob Buis
Comment 2 2012-07-11 14:29:30 PDT
(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.
Rob Buis
Comment 3 2012-07-11 14:30:47 PDT
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);
Yong Li
Comment 4 2012-07-11 15:02:27 PDT
(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.
Rob Buis
Comment 5 2012-07-11 15:20:10 PDT
Joe Mason
Comment 6 2012-07-12 07:45:04 PDT
(In reply to comment #4) > I prefer this solution to hacking the warning. Yes, I much prefer the more explicit solution.
Joe Mason
Comment 7 2012-07-12 07:45:43 PDT
Where does the test "#if (CPU(ARM) || CPU(MIPS)) && COMPILER(GCC)" come from, though?
Rob Buis
Comment 8 2012-07-12 07:53:53 PDT
(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.
Joe Mason
Comment 9 2012-07-12 07:56:51 PDT
(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.
Rob Buis
Comment 10 2012-07-12 08:02:42 PDT
(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.
Joe Mason
Comment 11 2012-07-12 08:15:19 PDT
(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?
Rob Buis
Comment 12 2012-07-12 09:59:02 PDT
Rob Buis
Comment 13 2012-07-12 10:27:19 PDT
Rob Buis
Comment 14 2012-07-17 08:24:09 PDT
Yong Li
Comment 15 2012-07-17 08:31:17 PDT
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()
Rob Buis
Comment 16 2012-07-17 15:31:45 PDT
Rob Buis
Comment 17 2012-07-17 15:33:59 PDT
(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...
Rob Buis
Comment 18 2012-07-18 08:07:37 PDT
Rob Buis
Comment 19 2012-07-18 10:18:10 PDT
Rob Buis
Comment 20 2012-07-18 11:51:57 PDT
Landed in r122990.
Note You need to log in before you can comment on or make changes to this bug.