Attachment 343898 Details for Bug 186536 – Patch

[patch] Patch

bug-186536-20180629110440.patch (text/plain), 9.05 KB, created by Frédéric Wang (:fredw) on 2018-06-29 02:04:41 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Frédéric Wang (:fredw)

Created: 2018-06-29 02:04:41 PDT

Size: 9.05 KB

patch

obsolete

>Subversion Revision: 233303
>diff --git a/Source/WTF/ChangeLog b/Source/WTF/ChangeLog
>index 326ba6f5c269be2b9cffb09b441d2ac8f6a3ea34..df3ec2d60019a7ea1cbaeaddbdac4b9ac4900545 100644
>--- a/Source/WTF/ChangeLog
>+++ b/Source/WTF/ChangeLog
>@@ -1,3 +1,30 @@
>+2018-06-29  Frederic Wang  <fwang@igalia.com>
>+
>+        WTF's internal std::optional implementation should abort() on bad optional access
>+        https://bugs.webkit.org/show_bug.cgi?id=186536
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Currently, some ports built with recent compilers will cause the program to abort when one
>+        tries to access the value of an unset std:optional (i.e. std::nullopt) as specified by C++17.
>+        However, most ports still use WTF's internal std::optional implementation, which does not
>+        verify illegal access. Hence it's not possible for developers working on these ports to
>+        detect issues like bugs #186189, #186535, #186752, #186753, #187139 or #187145. WTF's version
>+        of std::optional was introduced in bug #164199 but it was not possible to verify the
>+        availability of the value inside constexpr member functions because the ASSERT might involve
>+        asm declarations. This commit introduces a new ASSERT_UNDER_CONSTEXPR_CONTEXT macro (a
>+        simplified version of ASSERT that can be used in constexpr context) and uses it in
>+        WTF's implementation of std::optional.
>+
>+        * wtf/Assertions.h: Define ASSERT_UNDER_CONSTEXPR_CONTEXT as a version of ASSERT that can
>+        be used constexpr context (in particular avoids asm declarations).
>+        * wtf/Optional.h:
>+        (std::optional::operator ->): Add an assert to ensure the optional value is available.
>+        (std::optional::operator *): Ditto.
>+        (std::optional::value const): Ditto.
>+        (std::optional::value): Ditto.
>+        (std::optional<T::value const): Ditto.
>+
> 2018-06-27  Jonathan Bedard  <jbedard@apple.com>
> 
>         Enable WebKit iOS 12 build
>diff --git a/Source/WTF/wtf/Assertions.h b/Source/WTF/wtf/Assertions.h
>index 5e06eaa6e6199a229181368e0e70ecaa7dbd4f55..2c5d87ddb37fb1cb9d738904bbfbcb5f4a89f06a 100644
>--- a/Source/WTF/wtf/Assertions.h
>+++ b/Source/WTF/wtf/Assertions.h
>@@ -220,6 +220,12 @@ WTF_EXPORT_PRIVATE bool WTFIsDebuggerAttached(void);
> #define WTFBreakpointTrap() WTFCrash() // Not implemented.
> #endif
> 
>+#if COMPILER(MSVC)
>+#define WTFBreakpointTrapUnderConstexprContext() __debugbreak()
>+#else
>+#define WTFBreakpointTrapUnderConstexprContext() __builtin_trap()
>+#endif
>+
> #ifndef CRASH
> 
> #if defined(NDEBUG) && OS(DARWIN)
>@@ -287,6 +293,8 @@ WTF_EXPORT_PRIVATE NO_RETURN_DUE_TO_CRASH void WTFCrashWithSecurityImplication(v
> 
> #define ASSERT_UNUSED(variable, assertion) ((void)variable)
> 
>+#define ASSERT_UNDER_CONSTEXPR_CONTEXT(assertion) ((void)0)
>+
> #if ENABLE(SECURITY_ASSERTIONS)
> #define ASSERT_WITH_SECURITY_IMPLICATION(assertion) \
>     (!(assertion) ? \
>@@ -337,6 +345,18 @@ WTF_EXPORT_PRIVATE NO_RETURN_DUE_TO_CRASH void WTFCrashWithSecurityImplication(v
> 
> #define NO_RETURN_DUE_TO_ASSERT NO_RETURN_DUE_TO_CRASH
> 
>+/* ASSERT_UNDER_CONSTEXPR_CONTEXT
>+
>+   This is a special version of ASSERT(assertion) that can be used in constexpr context.
>+ */
>+#define ASSERT_UNDER_CONSTEXPR_CONTEXT(assertion) do { \
>+    if (!(assertion)) { \
>+        WTFReportAssertionFailure(__FILE__, __LINE__, WTF_PRETTY_FUNCTION, #assertion); \
>+        WTFBreakpointTrapUnderConstexprContext(); \
>+        __builtin_unreachable(); \
>+    } \
>+} while (0)
>+
> /* ASSERT_WITH_SECURITY_IMPLICATION
>  
>    Failure of this assertion indicates a possible security vulnerability.
>diff --git a/Source/WTF/wtf/Optional.h b/Source/WTF/wtf/Optional.h
>index 67ee27752ff908202b37ba85a71d1580f71b6101..d182dc8daa12137a920a3e8c8b416b883c16ea34 100644
>--- a/Source/WTF/wtf/Optional.h
>+++ b/Source/WTF/wtf/Optional.h
>@@ -530,8 +530,7 @@ public:
>   }
> 
>   OPTIONAL_MUTABLE_CONSTEXPR T* operator ->() {
>-    // FIXME: We need to offer special assert function that can be used under the contexpr context.
>-    // CONSTEXPR_ASSERT(initialized());
>+    ASSERT_UNDER_CONSTEXPR_CONTEXT(initialized());
>     return dataptr();
>   }
> 
>@@ -540,32 +539,27 @@ public:
>   }
> 
>   OPTIONAL_MUTABLE_CONSTEXPR T& operator *() & {
>-    // FIXME: We need to offer special assert function that can be used under the contexpr context.
>-    // CONSTEXPR_ASSERT(initialized());
>+    ASSERT_UNDER_CONSTEXPR_CONTEXT(initialized());
>     return contained_val();
>   }
> 
>   OPTIONAL_MUTABLE_CONSTEXPR T&& operator *() && {
>-    // FIXME: We need to offer special assert function that can be used under the contexpr context.
>-    // CONSTEXPR_ASSERT(initialized());
>+    ASSERT_UNDER_CONSTEXPR_CONTEXT(initialized());
>     return detail_::constexpr_move(contained_val());
>   }
> 
>   constexpr T const& value() const& {
>-    // FIXME: We need to offer special assert function that can be used under the contexpr context.
>-    // return initialized() ? contained_val() : (throw bad_optional_access("bad optional access"), contained_val());
>+    ASSERT_UNDER_CONSTEXPR_CONTEXT(initialized());
>     return contained_val();
>   }
> 
>   OPTIONAL_MUTABLE_CONSTEXPR T& value() & {
>-    // FIXME: We need to offer special assert function that can be used under the contexpr context.
>-    // return initialized() ? contained_val() : (throw bad_optional_access("bad optional access"), contained_val());
>+    ASSERT_UNDER_CONSTEXPR_CONTEXT(initialized());
>     return contained_val();
>   }
> 
>   OPTIONAL_MUTABLE_CONSTEXPR T&& value() && {
>-    // FIXME: We need to offer special assert function that can be used under the contexpr context.
>-    // if (!initialized()) __THROW_EXCEPTION(bad_optional_access("bad optional access"));
>+    ASSERT_UNDER_CONSTEXPR_CONTEXT(initialized());
>     return std::move(contained_val());
>   }
> 
>@@ -683,8 +677,7 @@ public:
>   }
> 
>   constexpr T& value() const {
>-    // FIXME: We need to offer special assert function that can be used under the contexpr context.
>-    // return ref ? *ref : (throw bad_optional_access("bad optional access"), *ref);
>+    ASSERT_UNDER_CONSTEXPR_CONTEXT(ref());
>     return *ref;
>   }
> 
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 0d1eff47fc90e567913564f7eb5d24ff6d48bceb..11d999208e906691c40cf1a99a66d9a1a26154cd 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,13 @@
>+2018-06-29  Frederic Wang  <fwang@igalia.com>
>+
>+        WTF's internal std::optional implementation should abort() on bad optional access
>+        https://bugs.webkit.org/show_bug.cgi?id=186536
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * TestExpectations: Add some flacky crashes for tests causing bad optional access.
>+        * platform/wpe/TestExpectations: Move this to the port-independent TestExpectations.
>+
> 2018-06-28  Dirk Schulze  <krit@webkit.org>
> 
>         [css-masking] Update clip-path box mapping to unified box
>diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
>index b0dbe775bc720dba9d196f792ce7cbf7ed40efdf..7478af4fd47c74b8a376cd134c44029b47c60fc3 100644
>--- a/LayoutTests/TestExpectations
>+++ b/LayoutTests/TestExpectations
>@@ -386,6 +386,13 @@ http/wpt/cross-origin-resource-policy/ [ Skip ]
> # End platform-specific tests.
> #//////////////////////////////////////////////////////////////////////////////////////////
> 
>+# These tests crash on platforms properly checking bad optional access.
>+# Since bug 186536, this is the case of debug builds using WTF's std::optional implementation.
>+webkit.org/b/187145 imported/w3c/web-platform-tests/web-animations/interfaces/Animatable/animate-no-browsing-context.html [ Pass Crash ]
>+webkit.org/b/186752 fast/css-grid-layout/flex-sizing-rows-min-max-height.html [ Pass Crash ]
>+webkit.org/b/186752 fast/css-grid-layout/grid-indefinite-size-auto-repeat-crash.html [ Pass Crash ]
>+webkit.org/b/186752 fast/css-grid-layout/maximize-tracks-definite-indefinite-height.html [ Pass Crash ]
>+
> # media/video-seek-after-end.html is flaky
> webkit.org/b/116293 media/video-seek-after-end.html [ Pass Failure ]
> 
>diff --git a/LayoutTests/platform/wpe/TestExpectations b/LayoutTests/platform/wpe/TestExpectations
>index fdec7e3bb7fe8eb8842202dad9d5638c462d8924..c4ce98e685d8aa65fc8ac5ccc1dca72476a913d2 100644
>--- a/LayoutTests/platform/wpe/TestExpectations
>+++ b/LayoutTests/platform/wpe/TestExpectations
>@@ -1220,10 +1220,6 @@ webkit.org/b/185254 http/tests/cache/disk-cache/redirect-chain-limits.html [ Fai
> webkit.org/b/184501 imported/w3c/web-platform-tests/WebCryptoAPI/generateKey/failures.worker.html [ Failure ]
> webkit.org/b/184501 imported/w3c/web-platform-tests/WebCryptoAPI/generateKey/successes.worker.html [ Failure ]
> 
>-webkit.org/b/186752 fast/css-grid-layout/flex-sizing-rows-min-max-height.html [ Crash ]
>-webkit.org/b/186752 fast/css-grid-layout/grid-indefinite-size-auto-repeat-crash.html [ Crash ]
>-webkit.org/b/186752 fast/css-grid-layout/maximize-tracks-definite-indefinite-height.html [ Crash ]
>-
> webkit.org/b/186100 css3/color-filters/color-filter-color-property-list-item.html [ ImageOnlyFailure ]
> webkit.org/b/186100 css3/color-filters/color-filter-ignore-semantic.html [ ImageOnlyFailure ]
> webkit.org/b/186100 css3/color-filters/color-filter-opacity.html [ ImageOnlyFailure ]

Actions: View | Formatted Diff | Diff

Attachments on bug 186536: 342475 | 342491 | 342505 | 343716 | 343763 | 343812 | 343813 | 343840 | 343898 | 344000 | 344007 | 344009 | 344010 | 344081 | 344412 | 344420