Bug 89787 – Alignment crash in MIMESniffer

• Component: Page Loading
• Status: RESOLVED FIXED

[Joe Mason]

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.

[Rob Buis]

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]

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

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]

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

Created attachment 151798 [details]
Patch

[Joe Mason]

(In reply to comment #4)
> I prefer this solution to hacking the warning.

Yes, I much prefer the more explicit solution.

[Joe Mason]

Where does the test "#if (CPU(ARM) || CPU(MIPS)) && COMPILER(GCC)" come from, though?

[Rob Buis]

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

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

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

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

Created attachment 151982 [details]
Patch

[Rob Buis]

Created attachment 151989 [details]
Patch

[Rob Buis]

Created attachment 152768 [details]
Patch

[Yong Li]

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]

Created attachment 152850 [details]
Patch

[Rob Buis]

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

Created attachment 153015 [details]
Patch

[Rob Buis]

Created attachment 153035 [details]
Patch

[Rob Buis]

Landed in r122990.