Changeset 205066 in webkit

Timestamp:

Aug 26, 2016 7:39:39 PM (8 years ago)

Author:

commit-queue@webkit.org

Message:

bitwise_cast uses inactive member of union
​https://bugs.webkit.org/show_bug.cgi?id=161244

Patch by JF Bastien <​jfbastien@apple.com> on 2016-08-26
Reviewed by Benjamin Poulain.

• wtf/Compiler.h:

Add COMPILER_HAS_CLANG_FEATURE

• wtf/StdLibExtras.h:

(WTF::bitwise_cast):
Fix C++ UB, add trivially-copyable check.

bitwise_cast stores into a union with one type and reads with
another, which is technically C++ undefined behavior because it's
accessing the wrong active member of the union. The better way to
do this is through memcpy, which compilers optimize as well
because it's known-size in known-not-to-escape storage (for small
types they'll inline and then convert stack memory access to SSA
values which may be in-register if that makes sense, which would
be a move between int/FP registers at worst).

The C++ Standard's section [basic.types] explicitly blesses memcpy:

For any trivially copyable type T, if two pointers to T point to
distinct T objects obj1 and obj2, where neither obj1 nor obj2 is a
base-class subobject, if the underlying bytes (1.7) making up obj1
are copied into obj2, 42 obj2 shall subsequently hold the same
value as obj1.

[Example:

T* t1p;
T* t2p;
provided that t2p points to an initialized object ...
std::memcpy(t1p, t2p, sizeof(T));
at this point, every subobject of trivially copyable type in *t1p contains
the same value as the corresponding subobject in *t2p

— end example ]

Whereas section [class.union] says:

In a union, at most one of the non-static data members can be
active at any time, that is, the value of at most one of the
non-static data members can be stored in a union at any time.

While we're at it, checking that sizeof(To) == sizeof(From) is
good, but we should also check that both types are trivially
copyable (can have a ctor, no dtor, and copy is defaulted as if by
memcpy for type and all subtypes). Unfortunately that trait isn't
implemented consistently in all recent compiler+stdlib
implementations, but recent clang has an equivalent builtin
(other compilers simply won't do the check, and will break on bots
with the right compilers which is better than the current silent
breakage). This builtin hack also avoids #include <type_traits>
which really doesn't save much.

Location:

trunk/Source/WTF

Files:

• ChangeLog (modified) (1 diff)
• wtf/Compiler.h (modified) (4 diffs)
• wtf/StdLibExtras.h (modified) (2 diffs)

trunk/Source/WTF/ChangeLog

@@ +1 @@
+2016-08-26  JF Bastien  <jfbastien@apple.com>
+
+        bitwise_cast uses inactive member of union
+        https://bugs.webkit.org/show_bug.cgi?id=161244
+
+        Reviewed by Benjamin Poulain.
+
+        * wtf/Compiler.h:
+        Add COMPILER_HAS_CLANG_FEATURE
+        * wtf/StdLibExtras.h:
+        (WTF::bitwise_cast):
+        Fix C++ UB, add trivially-copyable check.
+
+        bitwise_cast stores into a union with one type and reads with
+        another, which is technically C++ undefined behavior because it's
+        accessing the wrong active member of the union. The better way to
+        do this is through memcpy, which compilers optimize as well
+        because it's known-size in known-not-to-escape storage (for small
+        types they'll inline and then convert stack memory access to SSA
+        values which may be in-register if that makes sense, which would
+        be a move between int/FP registers at worst).
+
+        The C++ Standard's section [basic.types] explicitly blesses memcpy:
+
+          For any trivially copyable type T, if two pointers to T point to
+          distinct T objects obj1 and obj2, where neither obj1 nor obj2 is a
+          base-class subobject, if the underlying bytes (1.7) making up obj1
+          are copied into obj2, 42 obj2 shall subsequently hold the same
+          value as obj1.
+
+          [Example:
+            T* t1p;
+            T* t2p;
+            // provided that t2p points to an initialized object ...
+            std::memcpy(t1p, t2p, sizeof(T));
+            // at this point, every subobject of trivially copyable type in *t1p contains
+            // the same value as the corresponding subobject in *t2p
+          — end example ]
+
+        Whereas section [class.union] says:
+
+          In a union, at most one of the non-static data members can be
+          active at any time, that is, the value of at most one of the
+          non-static data members can be stored in a union at any time.
+
+        While we're at it, checking that sizeof(To) == sizeof(From) is
+        good, but we should also check that both types are trivially
+        copyable (can have a ctor, no dtor, and copy is defaulted as if by
+        memcpy for type and all subtypes). Unfortunately that trait isn't
+        implemented consistently in all recent compiler+stdlib
+        implementations, but recent clang has an equivalent builtin
+        (other compilers simply won't do the check, and will break on bots
+        with the right compilers which is better than the current silent
+        breakage). This builtin hack also avoids #include <type_traits>
+        which really doesn't save much.
+
 2016-08-26  Johan K. Jensen  <johan_jensen@apple.com>
 

trunk/Source/WTF/wtf/Compiler.h

@@ -36 +36 @@
 #define COMPILER_QUIRK(WTF_COMPILER_QUIRK) (defined WTF_COMPILER_QUIRK_##WTF_COMPILER_QUIRK  && WTF_COMPILER_QUIRK_##WTF_COMPILER_QUIRK)
 
-/* COMPILER_HAS_CLANG_BUILTIN() - wether the compiler supports a particular clang builtin. */
+/* COMPILER_HAS_CLANG_BUILTIN() - whether the compiler supports a particular clang builtin. */
 #ifdef __has_builtin
 #define COMPILER_HAS_CLANG_BUILTIN(x) __has_builtin(x)
@@ -43 +43 @@
 #endif
 
+/* COMPILER_HAS_CLANG_HEATURE() - whether the compiler supports a particular language or library feature. */
+/* http://clang.llvm.org/docs/LanguageExtensions.html#has-feature-and-has-extension */
+#ifdef __has_feature
+#define COMPILER_HAS_CLANG_FEATURE(x) __has_feature(x)
+#else
+#define COMPILER_HAS_CLANG_FEATURE(x) 0
+#endif
+
 /* ==== COMPILER() - primary detection of the compiler being used to build the project, in alphabetical order ==== */
 
@@ -49 +57 @@
 #if defined(__clang__)
 #define WTF_COMPILER_CLANG 1
-#define WTF_COMPILER_SUPPORTS_BLOCKS __has_feature(blocks)
-#define WTF_COMPILER_SUPPORTS_C_STATIC_ASSERT __has_feature(c_static_assert)
-#define WTF_COMPILER_SUPPORTS_CXX_REFERENCE_QUALIFIED_FUNCTIONS __has_feature(cxx_reference_qualified_functions)
-#define WTF_COMPILER_SUPPORTS_CXX_USER_LITERALS __has_feature(cxx_user_literals)
-#define WTF_COMPILER_SUPPORTS_FALLTHROUGH_WARNINGS __has_feature(cxx_attributes) && __has_warning("-Wimplicit-fallthrough")
-#define WTF_COMPILER_SUPPORTS_CXX_EXCEPTIONS __has_feature(cxx_exceptions)
+#define WTF_COMPILER_SUPPORTS_BLOCKS COMPILER_HAS_CLANG_FEATURE(blocks)
+#define WTF_COMPILER_SUPPORTS_C_STATIC_ASSERT COMPILER_HAS_CLANG_FEATURE(c_static_assert)
+#define WTF_COMPILER_SUPPORTS_CXX_REFERENCE_QUALIFIED_FUNCTIONS COMPILER_HAS_CLANG_FEATURE(cxx_reference_qualified_functions)
+#define WTF_COMPILER_SUPPORTS_CXX_USER_LITERALS COMPILER_HAS_CLANG_FEATURE(cxx_user_literals)
+#define WTF_COMPILER_SUPPORTS_FALLTHROUGH_WARNINGS COMPILER_HAS_CLANG_FEATURE(cxx_attributes) && __has_warning("-Wimplicit-fallthrough")
+#define WTF_COMPILER_SUPPORTS_CXX_EXCEPTIONS COMPILER_HAS_CLANG_FEATURE(cxx_exceptions)
+#define WTF_COMPILER_SUPPORTS_BUILTIN_IS_TRIVIALLY_COPYABLE COMPILER_HAS_CLANG_FEATURE(is_trivially_copyable)
 
 #ifdef __cplusplus
@@ -136 +145 @@
 #endif
 
-#if defined(__has_feature)
-#define ASAN_ENABLED __has_feature(address_sanitizer)
-#else
-#define ASAN_ENABLED 0
-#endif
+#define ASAN_ENABLED COMPILER_HAS_CLANG_FEATURE(address_sanitizer)
 
 #if ASAN_ENABLED

trunk/Source/WTF/wtf/StdLibExtras.h

@@ -29 +29 @@
 
 #include <chrono>
+#include <cstring>
 #include <memory>
-#include <string.h>
 #include <wtf/Assertions.h>
 #include <wtf/CheckedArithmetic.h>
+#include <wtf/Compiler.h>
 
 // This was used to declare and define a static local variable (static T;) so that
@@ -146 +147 @@
 {
     static_assert(sizeof(FromType) == sizeof(ToType), "bitwise_cast size of FromType and ToType must be equal!");
-    union {
-        FromType from;
-        ToType to;
-    } u;
-    u.from = from;
-    return u.to;
+#if COMPILER_SUPPORTS(BUILTIN_IS_TRIVIALLY_COPYABLE)
+    // Not all recent STL implementations support the std::is_trivially_copyable type trait. Work around this by only checking on toolchains which have the equivalent compiler intrinsic.
+    static_assert(__is_trivially_copyable(ToType), "bitwise_cast of non-trivially-copyable type!");
+    static_assert(__is_trivially_copyable(FromType), "bitwise_cast of non-trivially-copyable type!");
+#endif
+    ToType to;
+    std::memcpy(&to, &from, sizeof(to));
+    return to;
 }