Bug 89787 - Alignment crash in MIMESniffer
Summary: Alignment crash in MIMESniffer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-22 14:53 PDT by Joe Mason
Modified: 2012-07-18 11:51 PDT (History)
6 users (show)

See Also:


Attachments
Patch (1.65 KB, patch)
2012-07-11 15:20 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (2.25 KB, patch)
2012-07-12 09:59 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (2.21 KB, patch)
2012-07-12 10:27 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (3.54 KB, patch)
2012-07-17 08:24 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (3.54 KB, patch)
2012-07-17 15:31 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (3.45 KB, patch)
2012-07-18 08:07 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (3.43 KB, patch)
2012-07-18 10:18 PDT, Rob Buis
yong.li.webkit: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joe Mason 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.
Comment 1 Rob Buis 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.
Comment 2 Rob Buis 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.
Comment 3 Rob Buis 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);
Comment 4 Yong Li 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.
Comment 5 Rob Buis 2012-07-11 15:20:10 PDT
Created attachment 151798 [details]
Patch
Comment 6 Joe Mason 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.
Comment 7 Joe Mason 2012-07-12 07:45:43 PDT
Where does the test "#if (CPU(ARM) || CPU(MIPS)) && COMPILER(GCC)" come from, though?
Comment 8 Rob Buis 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.
Comment 9 Joe Mason 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.
Comment 10 Rob Buis 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.
Comment 11 Joe Mason 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?
Comment 12 Rob Buis 2012-07-12 09:59:02 PDT
Created attachment 151982 [details]
Patch
Comment 13 Rob Buis 2012-07-12 10:27:19 PDT
Created attachment 151989 [details]
Patch
Comment 14 Rob Buis 2012-07-17 08:24:09 PDT
Created attachment 152768 [details]
Patch
Comment 15 Yong Li 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()
Comment 16 Rob Buis 2012-07-17 15:31:45 PDT
Created attachment 152850 [details]
Patch
Comment 17 Rob Buis 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...
Comment 18 Rob Buis 2012-07-18 08:07:37 PDT
Created attachment 153015 [details]
Patch
Comment 19 Rob Buis 2012-07-18 10:18:10 PDT
Created attachment 153035 [details]
Patch
Comment 20 Rob Buis 2012-07-18 11:51:57 PDT
Landed in r122990.