Attachment 358729 Details for Bug 193292 – proposed patch.

[patch] proposed patch.

bug-193292.patch (text/plain), 12.07 KB, created by Mark Lam on 2019-01-09 13:05:00 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Mark Lam

Created: 2019-01-09 13:05:00 PST

Size: 12.07 KB

patch

obsolete

>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 239781)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,14 @@
>+2019-01-09  Mark Lam  <mark.lam@apple.com>
>+
>+        Gigacage disabling checks should handle the GIGACAGE_ALLOCATION_CAN_FAIL case properly.
>+        https://bugs.webkit.org/show_bug.cgi?id=193292
>+        <rdar://problem/46485450>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * runtime/VM.h:
>+        (JSC::VM::gigacageAuxiliarySpace):
>+
> 2019-01-08  Keith Miller  <keith_miller@apple.com>
> 
>         builtins should be able to use async/await
>Index: Source/JavaScriptCore/runtime/VM.h
>===================================================================
>--- Source/JavaScriptCore/runtime/VM.h	(revision 239781)
>+++ Source/JavaScriptCore/runtime/VM.h	(working copy)
>@@ -1,5 +1,5 @@
> /*
>- * Copyright (C) 2008-2018 Apple Inc. All rights reserved.
>+ * Copyright (C) 2008-2019 Apple Inc. All rights reserved.
>  *
>  * Redistribution and use in source and binary forms, with or without
>  * modification, are permitted provided that the following conditions
>@@ -346,6 +346,8 @@ public:
>     ALWAYS_INLINE CompleteSubspace& gigacageAuxiliarySpace(Gigacage::Kind kind)
>     {
>         switch (kind) {
>+        case Gigacage::ReservedForFlagsAndNotABasePtr:
>+            RELEASE_ASSERT_NOT_REACHED();
>         case Gigacage::Primitive:
>             return primitiveGigacageAuxiliarySpace;
>         case Gigacage::JSValue:
>Index: Source/WTF/ChangeLog
>===================================================================
>--- Source/WTF/ChangeLog	(revision 239781)
>+++ Source/WTF/ChangeLog	(working copy)
>@@ -1,3 +1,15 @@
>+2019-01-09  Mark Lam  <mark.lam@apple.com>
>+
>+        Gigacage disabling checks should handle the GIGACAGE_ALLOCATION_CAN_FAIL case properly.
>+        https://bugs.webkit.org/show_bug.cgi?id=193292
>+        <rdar://problem/46485450>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Update the USE_SYSTEM_MALLOC version of Gigacage.h to match the bmalloc version.
>+
>+        * wtf/Gigacage.h:
>+
> 2019-01-07  David Kilzer  <ddkilzer@apple.com>
> 
>         Prefer RetainPtr<NSObject> to RetainPtr<NSObject *>
>Index: Source/WTF/wtf/Gigacage.h
>===================================================================
>--- Source/WTF/wtf/Gigacage.h	(revision 239781)
>+++ Source/WTF/wtf/Gigacage.h	(working copy)
>@@ -1,5 +1,5 @@
> /*
>- * Copyright (C) 2017-2018 Apple Inc. All rights reserved.
>+ * Copyright (C) 2017-2019 Apple Inc. All rights reserved.
>  *
>  * Redistribution and use in source and binary forms, with or without
>  * modification, are permitted provided that the following conditions
>@@ -40,15 +40,20 @@ alignas(void*) extern WTF_EXPORT_PRIVATE
> namespace Gigacage {
> 
> struct BasePtrs {
>+    uintptr_t reservedForFlags;
>     void* primitive;
>     void* jsValue;
> };
> 
> enum Kind {
>+    ReservedForFlagsAndNotABasePtr = 0,
>     Primitive,
>     JSValue,
> };
> 
>+static_assert(offsetof(BasePtrs, primitive) == Kind::Primitive * sizeof(void*), "");
>+static_assert(offsetof(BasePtrs, jsValue) == Kind::JSValue * sizeof(void*), "");
>+
> inline void ensureGigacage() { }
> inline void disablePrimitiveGigacage() { }
> inline bool shouldBeEnabled() { return false; }
>@@ -65,6 +70,8 @@ inline bool canPrimitiveGigacageBeDisabl
> ALWAYS_INLINE const char* name(Kind kind)
> {
>     switch (kind) {
>+    case ReservedForFlagsAndNotABasePtr:
>+        RELEASE_ASSERT_NOT_REACHED();
>     case Primitive:
>         return "Primitive";
>     case JSValue:
>@@ -77,6 +84,8 @@ ALWAYS_INLINE const char* name(Kind kind
> ALWAYS_INLINE void*& basePtr(BasePtrs& basePtrs, Kind kind)
> {
>     switch (kind) {
>+    case ReservedForFlagsAndNotABasePtr:
>+        RELEASE_ASSERT_NOT_REACHED();
>     case Primitive:
>         return basePtrs.primitive;
>     case JSValue:
>Index: Source/bmalloc/ChangeLog
>===================================================================
>--- Source/bmalloc/ChangeLog	(revision 239781)
>+++ Source/bmalloc/ChangeLog	(working copy)
>@@ -1,3 +1,50 @@
>+2019-01-09  Mark Lam  <mark.lam@apple.com>
>+
>+        Gigacage disabling checks should handle the GIGACAGE_ALLOCATION_CAN_FAIL case properly.
>+        https://bugs.webkit.org/show_bug.cgi?id=193292
>+        <rdar://problem/46485450>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Previously, when GIGACAGE_ALLOCATION_CAN_FAIL is true, we allow the Gigacage to
>+        be disabled if we fail to allocate memory for it.  However, addPrimitiveDisableCallback()
>+        still always assumes that the Gigacage is always enabled after ensureGigacage() is
>+        called.
>+
>+        This patch updates addPrimitiveDisableCallback() to allow the Gigacage to already
>+        be disabled if GIGACAGE_ALLOCATION_CAN_FAIL is true and wasEnabled() is false.
>+
>+        In this patch, we also put the wasEnabled flag in the 0th slot of the
>+        g_gigacageBasePtrs buffer to ensure that it is also protected against writes just
>+        like the Gigacage base pointers.
>+
>+        To achieve this, we do the following:
>+        1. Added a reservedForFlags field in struct BasePtrs.
>+        2. Added a ReservedForFlagsAndNotABasePtr Gigacage::Kind.
>+        3. Added assertions to ensure that the BasePtrs::primitive is at the offset
>+           matching the offset computed from Gigacage::Primitive.  Ditto for
>+           BasePtrs::jsValue and Gigacage::JSValue.
>+        4. Added assertions to ensure that Gigacage::ReservedForFlagsAndNotABasePtr is not
>+           used for fetching a Gigacage base pointer.
>+        5. Added RELEASE_BASSERT_NOT_REACHED() to implement such assertions in bmalloc.
>+
>+        No test added because this issue requires Gigacage allocation to fail in order to
>+        manifest.  I've tested it manually by modifying the code locally to force an
>+        allocation failure.
>+
>+        * bmalloc/BAssert.h:
>+        * bmalloc/Gigacage.cpp:
>+        (Gigacage::ensureGigacage):
>+        (Gigacage::addPrimitiveDisableCallback):
>+        * bmalloc/Gigacage.h:
>+        (Gigacage::wasEnabled):
>+        (Gigacage::setWasEnabled):
>+        (Gigacage::name):
>+        (Gigacage::basePtr):
>+        (Gigacage::size):
>+        * bmalloc/HeapKind.h:
>+        (bmalloc::heapKind):
>+
> 2018-12-15  Yusuke Suzuki  <yusukesuzuki@slowstart.org>
> 
>         Unreviewed, suppress warnings in Linux
>Index: Source/bmalloc/bmalloc/BAssert.h
>===================================================================
>--- Source/bmalloc/bmalloc/BAssert.h	(revision 239781)
>+++ Source/bmalloc/bmalloc/BAssert.h	(working copy)
>@@ -81,6 +81,7 @@
> } while (0)
> 
> #define RELEASE_BASSERT(x) BASSERT_IMPL(x)
>+#define RELEASE_BASSERT_NOT_REACHED() BCRASH()
> 
> #if BUSE(OS_LOG)
> #define BMALLOC_LOGGING_PREFIX "bmalloc: "
>Index: Source/bmalloc/bmalloc/Gigacage.cpp
>===================================================================
>--- Source/bmalloc/bmalloc/Gigacage.cpp	(revision 239781)
>+++ Source/bmalloc/bmalloc/Gigacage.cpp	(working copy)
>@@ -1,5 +1,5 @@
> /*
>- * Copyright (C) 2017-2018 Apple Inc. All rights reserved.
>+ * Copyright (C) 2017-2019 Apple Inc. All rights reserved.
>  *
>  * Redistribution and use in source and binary forms, with or without
>  * modification, are permitted provided that the following conditions
>@@ -42,14 +42,16 @@
> // If this were less than 32GB, those OOB accesses could reach outside of the cage.
> #define GIGACAGE_RUNWAY (32llu * 1024 * 1024 * 1024)
> 
>+// Note: g_gigacageBasePtrs[0] is reserved for storing the wasEnabled flag.
>+// The first gigacageBasePtr will start at g_gigacageBasePtrs[sizeof(void*)].
>+// This is done so that the wasEnabled flag will also be protected along with the
>+// gigacageBasePtrs.
> alignas(GIGACAGE_BASE_PTRS_SIZE) char g_gigacageBasePtrs[GIGACAGE_BASE_PTRS_SIZE];
> 
> using namespace bmalloc;
> 
> namespace Gigacage {
> 
>-bool g_wasEnabled;
>-
> namespace {
> 
> bool s_isDisablingPrimitiveGigacageDisabled;
>@@ -103,6 +105,8 @@ struct PrimitiveDisableCallbacks {
> size_t runwaySize(Kind kind)
> {
>     switch (kind) {
>+    case Kind::ReservedForFlagsAndNotABasePtr:
>+        RELEASE_BASSERT_NOT_REACHED();
>     case Kind::Primitive:
>         return static_cast<size_t>(GIGACAGE_RUNWAY);
>     case Kind::JSValue:
>@@ -126,7 +130,7 @@ void ensureGigacage()
>             
>             Kind shuffledKinds[numKinds];
>             for (unsigned i = 0; i < numKinds; ++i)
>-                shuffledKinds[i] = static_cast<Kind>(i);
>+                shuffledKinds[i] = static_cast<Kind>(i + 1); // + 1 to skip Kind::ReservedForFlagsAndNotABasePtr.
>             
>             // We just go ahead and assume that 64 bits is enough randomness. That's trivially true right
>             // now, but would stop being true if we went crazy with gigacages. Based on my math, 21 is the
>@@ -182,8 +186,8 @@ void ensureGigacage()
>             }
>             
>             vmDeallocatePhysicalPages(base, totalSize);
>+            setWasEnabled();
>             protectGigacageBasePtrs();
>-            g_wasEnabled = true;
>         });
> #endif // GIGACAGE_ENABLED
> }
>@@ -211,7 +215,8 @@ void addPrimitiveDisableCallback(void (*
>     ensureGigacage();
>     if (!basePtrs().primitive) {
>         // It was already disabled or we were never able to enable it.
>-        function(argument);
>+        if (!GIGACAGE_ALLOCATION_CAN_FAIL || wasEnabled())
>+            function(argument);
>         return;
>     }
>     
>Index: Source/bmalloc/bmalloc/Gigacage.h
>===================================================================
>--- Source/bmalloc/bmalloc/Gigacage.h	(revision 239781)
>+++ Source/bmalloc/bmalloc/Gigacage.h	(working copy)
>@@ -1,5 +1,5 @@
> /*
>- * Copyright (C) 2017 Apple Inc. All rights reserved.
>+ * Copyright (C) 2017-2019 Apple Inc. All rights reserved.
>  *
>  * Redistribution and use in source and binary forms, with or without
>  * modification, are permitted provided that the following conditions
>@@ -75,19 +75,24 @@ extern "C" alignas(GIGACAGE_BASE_PTRS_SI
> 
> namespace Gigacage {
> 
>-extern BEXPORT bool g_wasEnabled;
>-BINLINE bool wasEnabled() { return g_wasEnabled; }
>+BINLINE bool wasEnabled() { return g_gigacageBasePtrs[0]; }
>+BINLINE void setWasEnabled() { g_gigacageBasePtrs[0] = true; }
> 
> struct BasePtrs {
>+    uintptr_t reservedForFlags;
>     void* primitive;
>     void* jsValue;
> };
> 
> enum Kind {
>+    ReservedForFlagsAndNotABasePtr = 0,
>     Primitive,
>     JSValue,
> };
> 
>+static_assert(offsetof(BasePtrs, primitive) == Kind::Primitive * sizeof(void*), "");
>+static_assert(offsetof(BasePtrs, jsValue) == Kind::JSValue * sizeof(void*), "");
>+
> static constexpr unsigned numKinds = 2;
> 
> BEXPORT void ensureGigacage();
>@@ -107,6 +112,8 @@ inline bool canPrimitiveGigacageBeDisabl
> BINLINE const char* name(Kind kind)
> {
>     switch (kind) {
>+    case ReservedForFlagsAndNotABasePtr:
>+        RELEASE_BASSERT_NOT_REACHED();
>     case Primitive:
>         return "Primitive";
>     case JSValue:
>@@ -119,6 +126,8 @@ BINLINE const char* name(Kind kind)
> BINLINE void*& basePtr(BasePtrs& basePtrs, Kind kind)
> {
>     switch (kind) {
>+    case ReservedForFlagsAndNotABasePtr:
>+        RELEASE_BASSERT_NOT_REACHED();
>     case Primitive:
>         return basePtrs.primitive;
>     case JSValue:
>@@ -146,6 +155,8 @@ BINLINE bool isEnabled(Kind kind)
> BINLINE size_t size(Kind kind)
> {
>     switch (kind) {
>+    case ReservedForFlagsAndNotABasePtr:
>+        RELEASE_BASSERT_NOT_REACHED();
>     case Primitive:
>         return static_cast<size_t>(PRIMITIVE_GIGACAGE_SIZE);
>     case JSValue:
>Index: Source/bmalloc/bmalloc/HeapKind.h
>===================================================================
>--- Source/bmalloc/bmalloc/HeapKind.h	(revision 239781)
>+++ Source/bmalloc/bmalloc/HeapKind.h	(working copy)
>@@ -1,5 +1,5 @@
> /*
>- * Copyright (C) 2017 Apple Inc. All rights reserved.
>+ * Copyright (C) 2017-2019 Apple Inc. All rights reserved.
>  *
>  * Redistribution and use in source and binary forms, with or without
>  * modification, are permitted provided that the following conditions
>@@ -70,6 +70,8 @@ BINLINE Gigacage::Kind gigacageKind(Heap
> BINLINE HeapKind heapKind(Gigacage::Kind kind)
> {
>     switch (kind) {
>+    case Gigacage::ReservedForFlagsAndNotABasePtr:
>+        RELEASE_BASSERT_NOT_REACHED();
>     case Gigacage::Primitive:
>         return HeapKind::PrimitiveGigacage;
>     case Gigacage::JSValue:

Flags:

mark.lam: review-

Actions: View | Formatted Diff | Diff

Attachments on bug 193292: 358729 | 358741