ObjectToStringAdaptiveInferredPropertyValueWatchpoint should not reinstall itself...
authormark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 25 May 2017 17:03:13 +0000 (17:03 +0000)
committermark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 25 May 2017 17:03:13 +0000 (17:03 +0000)
https://bugs.webkit.org/show_bug.cgi?id=172548
<rdar://problem/31458393>

Reviewed by Filip Pizlo.

JSTests:

* stress/regress-172548.patch: Added.

Source/JavaScriptCore:

Consider the following scenario:

1. A ObjectToStringAdaptiveInferredPropertyValueWatchpoint O1, watches for
   structure transitions, e.g. structure S2 transitioning to structure S3.
   In this case, O1 would be installed in S2's watchpoint set.
2. When the structure transition happens, structure S2 will fire watchpoint O1.
3. O1's handler will normally re-install itself in the watchpoint set of the new
   "transitioned to" structure S3.
4. "Installation" here requires writing into the StructureRareData SD3 of the new
   structure S3.  If SD3 does not exist yet, the installation process will trigger
   the allocation of StructureRareData SD3.
5. It is possible that the Structure S1, and StructureRareData SD1 that owns the
   ObjectToStringAdaptiveInferredPropertyValueWatchpoint O1 is no longer reachable
   by the GC, and therefore will be collected soon.
6. The allocation of SD3 in (4) may trigger the sweeping of the StructureRareData
   SD1.  This, in turn, triggers the deletion of the
   ObjectToStringAdaptiveInferredPropertyValueWatchpoint O1.

After O1 is deleted in (6) and SD3 is allocated in (4), execution continues in
AdaptiveInferredPropertyValueWatchpointBase::fire() where O1 gets installed in
structure S3's watchpoint set.  This is obviously incorrect because O1 is already
deleted.  The result is that badness happens later when S3's watchpoint set fires
its watchpoints and accesses the deleted O1.

The fix is to enhance AdaptiveInferredPropertyValueWatchpointBase::fire() to
check if "this" is still valid before proceeding to re-install itself or to
invoke its handleFire() method.

ObjectToStringAdaptiveInferredPropertyValueWatchpoint (which extends
AdaptiveInferredPropertyValueWatchpointBase) will override its isValid() method,
and return false its owner StructureRareData is no longer reachable by the GC.
This ensures that it won't be deleted while it's installed to any watchpoint set.

Additional considerations and notes:
1. In the above, I talked about the ObjectToStringAdaptiveInferredPropertyValueWatchpoint
   being installed in watchpoint sets.  What actually happens is that
   ObjectToStringAdaptiveInferredPropertyValueWatchpoint has 2 members
   (m_structureWatchpoint and m_propertyWatchpoint) which may be installed in
   watchpoint sets.  The ObjectToStringAdaptiveInferredPropertyValueWatchpoint is
   not itself a Watchpoint object.

   But for brevity, in the above, I refer to the ObjectToStringAdaptiveInferredPropertyValueWatchpoint
   instead of its Watchpoint members.  The description of the issue is still
   accurate given the life-cycle of the Watchpoint members are embedded in the
   enclosing ObjectToStringAdaptiveInferredPropertyValueWatchpoint object, and
   hence, they share the same life-cycle.

2. The top of AdaptiveInferredPropertyValueWatchpointBase::fire() removes its
   m_structureWatchpoint and m_propertyWatchpoint if they have been added to any
   watchpoint sets.  This is safe to do even if the owner StructureRareData is no
   longer reachable by the GC.

   This is because the only way we can get to AdaptiveInferredPropertyValueWatchpointBase::fire()
   is if its Watchpoint members are still installed in some watchpoint set that
   fired.  This means that the AdaptiveInferredPropertyValueWatchpointBase
   instance has not been deleted yet, because its destructor will automatically
   remove the Watchpoint members from any watchpoint sets.

* bytecode/AdaptiveInferredPropertyValueWatchpointBase.cpp:
(JSC::AdaptiveInferredPropertyValueWatchpointBase::fire):
(JSC::AdaptiveInferredPropertyValueWatchpointBase::isValid):
* bytecode/AdaptiveInferredPropertyValueWatchpointBase.h:
* heap/FreeList.cpp:
(JSC::FreeList::contains):
* heap/FreeList.h:
* heap/HeapCell.h:
* heap/HeapCellInlines.h:
(JSC::HeapCell::isLive):
* heap/MarkedAllocator.h:
(JSC::MarkedAllocator::isFreeListedCell):
* heap/MarkedBlock.h:
* heap/MarkedBlockInlines.h:
(JSC::MarkedBlock::Handle::isFreeListedCell):
* runtime/StructureRareData.cpp:
(JSC::ObjectToStringAdaptiveInferredPropertyValueWatchpoint::isValid):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@217429 268f45cc-cd09-0410-ab3c-d52691b4dbfc

13 files changed:
JSTests/ChangeLog
JSTests/stress/regress-172548.patch [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/AdaptiveInferredPropertyValueWatchpointBase.cpp
Source/JavaScriptCore/bytecode/AdaptiveInferredPropertyValueWatchpointBase.h
Source/JavaScriptCore/heap/FreeList.cpp
Source/JavaScriptCore/heap/FreeList.h
Source/JavaScriptCore/heap/HeapCell.h
Source/JavaScriptCore/heap/HeapCellInlines.h
Source/JavaScriptCore/heap/MarkedAllocator.h
Source/JavaScriptCore/heap/MarkedBlock.h
Source/JavaScriptCore/heap/MarkedBlockInlines.h
Source/JavaScriptCore/runtime/StructureRareData.cpp

index c452d23..3eacf08 100644 (file)
@@ -1,3 +1,13 @@
+2017-05-25  Mark Lam  <mark.lam@apple.com>
+
+        ObjectToStringAdaptiveInferredPropertyValueWatchpoint should not reinstall itself nor handleFire if it's dying shortly.
+        https://bugs.webkit.org/show_bug.cgi?id=172548
+        <rdar://problem/31458393>
+
+        Reviewed by Filip Pizlo.
+
+        * stress/regress-172548.patch: Added.
+
 2017-05-23  Saam Barati  <sbarati@apple.com>
 
         We should not mmap zero bytes for a memory in Wasm
diff --git a/JSTests/stress/regress-172548.patch b/JSTests/stress/regress-172548.patch
new file mode 100644 (file)
index 0000000..a2f7331
--- /dev/null
@@ -0,0 +1,43 @@
+//@ run("custom-noCJIT-noConcurrentGC-noGenGC-noDFG", "--useConcurrentJIT=false", "--useConcurrentGC=false", "--useGenerationalGC=false", "--useDFGJIT=false")
+
+// This test should not crash.
+
+(function() {
+    function foo(x, y) {
+        return (( ~ (((((Math.fround(0 % y) >= y) ) / ((Math.max(((Math.atan2(((Math.pow((-Number.MAX_VALUE >>> 0), (x | 0)) | 0) >>> 0), y) >>> 0) - ( + (( ! (0 | 0)) | 0))), Math.pow(Math.fround(((x | 0) ^ y)), ( ! ( + y)))) | 0) | 0)) | 0) | 0)) | 0);
+    };
+
+    var inputs = [null, NaN, (1/0), NaN, (1/0), arguments.callee, null, (1/0), arguments.callee, NaN, (1/0), arguments.callee, (1/0), arguments.callee, NaN, null, NaN, null, (1/0), NaN, arguments.callee, (1/0), NaN, null, arguments.callee, (1/0), NaN, NaN, NaN, NaN, NaN, arguments.callee, null, (1/0), NaN, null, null, (1/0), (1/0), NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN, null, null, (1/0), NaN, null, null, arguments.callee, NaN, NaN, null, null, arguments.callee, null, NaN, (1/0), NaN, NaN, arguments.callee, NaN, arguments.callee, null, NaN, NaN, null, arguments.callee, NaN, null, (1/0), NaN, arguments.callee, null, null, NaN, null, NaN, arguments.callee, arguments.callee, arguments.callee, arguments.callee, null, arguments.callee, (1/0), (1/0), (1/0), (1/0), (1/0), NaN, (1/0), arguments.callee, NaN, (1/0)];
+
+    for (var j = 0; j < inputs.length; ++j) {       
+        for (var k = 0; k < inputs.length; ++k) {         
+            foo(inputs[k], inputs[k]);
+        }     
+    }   
+
+})();
+
+Math.ceil = "\uEB0D";
+
+m2 = new Map;
+m2.toSource = (function() { });
+
+m1 = new Map;
+m2.toString();
+
+o2 = m1.__proto__;
+m2 = new Map;
+
+test = function() {
+    Math.log1p(/x/g), Math.log1p(/x/g), Math.ceil(0x100000001);
+};
+
+for (var i = 0; i < 33000; i++) {
+    try {
+        test();
+    } catch (e) {
+    }
+}
+
+o2.g0 = this;
+o2.h1 = {};
index 829d1da..fb8a734 100644 (file)
@@ -1,3 +1,87 @@
+2017-05-25  Mark Lam  <mark.lam@apple.com>
+
+        ObjectToStringAdaptiveInferredPropertyValueWatchpoint should not reinstall itself nor handleFire if it's dying shortly.
+        https://bugs.webkit.org/show_bug.cgi?id=172548
+        <rdar://problem/31458393>
+
+        Reviewed by Filip Pizlo.
+
+        Consider the following scenario:
+
+        1. A ObjectToStringAdaptiveInferredPropertyValueWatchpoint O1, watches for
+           structure transitions, e.g. structure S2 transitioning to structure S3.
+           In this case, O1 would be installed in S2's watchpoint set.
+        2. When the structure transition happens, structure S2 will fire watchpoint O1.
+        3. O1's handler will normally re-install itself in the watchpoint set of the new
+           "transitioned to" structure S3.
+        4. "Installation" here requires writing into the StructureRareData SD3 of the new
+           structure S3.  If SD3 does not exist yet, the installation process will trigger
+           the allocation of StructureRareData SD3.
+        5. It is possible that the Structure S1, and StructureRareData SD1 that owns the
+           ObjectToStringAdaptiveInferredPropertyValueWatchpoint O1 is no longer reachable
+           by the GC, and therefore will be collected soon.
+        6. The allocation of SD3 in (4) may trigger the sweeping of the StructureRareData
+           SD1.  This, in turn, triggers the deletion of the
+           ObjectToStringAdaptiveInferredPropertyValueWatchpoint O1.
+
+        After O1 is deleted in (6) and SD3 is allocated in (4), execution continues in
+        AdaptiveInferredPropertyValueWatchpointBase::fire() where O1 gets installed in
+        structure S3's watchpoint set.  This is obviously incorrect because O1 is already
+        deleted.  The result is that badness happens later when S3's watchpoint set fires
+        its watchpoints and accesses the deleted O1.
+
+        The fix is to enhance AdaptiveInferredPropertyValueWatchpointBase::fire() to
+        check if "this" is still valid before proceeding to re-install itself or to
+        invoke its handleFire() method.
+
+        ObjectToStringAdaptiveInferredPropertyValueWatchpoint (which extends
+        AdaptiveInferredPropertyValueWatchpointBase) will override its isValid() method,
+        and return false its owner StructureRareData is no longer reachable by the GC.
+        This ensures that it won't be deleted while it's installed to any watchpoint set.
+
+        Additional considerations and notes:
+        1. In the above, I talked about the ObjectToStringAdaptiveInferredPropertyValueWatchpoint
+           being installed in watchpoint sets.  What actually happens is that
+           ObjectToStringAdaptiveInferredPropertyValueWatchpoint has 2 members
+           (m_structureWatchpoint and m_propertyWatchpoint) which may be installed in
+           watchpoint sets.  The ObjectToStringAdaptiveInferredPropertyValueWatchpoint is
+           not itself a Watchpoint object.
+
+           But for brevity, in the above, I refer to the ObjectToStringAdaptiveInferredPropertyValueWatchpoint
+           instead of its Watchpoint members.  The description of the issue is still
+           accurate given the life-cycle of the Watchpoint members are embedded in the
+           enclosing ObjectToStringAdaptiveInferredPropertyValueWatchpoint object, and
+           hence, they share the same life-cycle.
+
+        2. The top of AdaptiveInferredPropertyValueWatchpointBase::fire() removes its
+           m_structureWatchpoint and m_propertyWatchpoint if they have been added to any
+           watchpoint sets.  This is safe to do even if the owner StructureRareData is no
+           longer reachable by the GC.
+
+           This is because the only way we can get to AdaptiveInferredPropertyValueWatchpointBase::fire()
+           is if its Watchpoint members are still installed in some watchpoint set that
+           fired.  This means that the AdaptiveInferredPropertyValueWatchpointBase
+           instance has not been deleted yet, because its destructor will automatically
+           remove the Watchpoint members from any watchpoint sets.
+
+        * bytecode/AdaptiveInferredPropertyValueWatchpointBase.cpp:
+        (JSC::AdaptiveInferredPropertyValueWatchpointBase::fire):
+        (JSC::AdaptiveInferredPropertyValueWatchpointBase::isValid):
+        * bytecode/AdaptiveInferredPropertyValueWatchpointBase.h:
+        * heap/FreeList.cpp:
+        (JSC::FreeList::contains):
+        * heap/FreeList.h:
+        * heap/HeapCell.h:
+        * heap/HeapCellInlines.h:
+        (JSC::HeapCell::isLive):
+        * heap/MarkedAllocator.h:
+        (JSC::MarkedAllocator::isFreeListedCell):
+        * heap/MarkedBlock.h:
+        * heap/MarkedBlockInlines.h:
+        (JSC::MarkedBlock::Handle::isFreeListedCell):
+        * runtime/StructureRareData.cpp:
+        (JSC::ObjectToStringAdaptiveInferredPropertyValueWatchpoint::isValid):
+
 2017-05-23  Saam Barati  <sbarati@apple.com>
 
         We should not mmap zero bytes for a memory in Wasm
index 2fd7031..3f02d4f 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2015 Apple Inc. All rights reserved.
+ * Copyright (C) 2015-2017 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -57,6 +57,9 @@ void AdaptiveInferredPropertyValueWatchpointBase::fire(const FireDetail& detail)
     if (m_propertyWatchpoint.isOnList())
         m_propertyWatchpoint.remove();
 
+    if (!isValid())
+        return;
+
     if (m_key.isWatchable(PropertyCondition::EnsureWatchability)) {
         install();
         return;
@@ -65,6 +68,11 @@ void AdaptiveInferredPropertyValueWatchpointBase::fire(const FireDetail& detail)
     handleFire(detail);
 }
 
+bool AdaptiveInferredPropertyValueWatchpointBase::isValid() const
+{
+    return true;
+}
+
 void AdaptiveInferredPropertyValueWatchpointBase::StructureWatchpoint::fireInternal(const FireDetail& detail)
 {
     ptrdiff_t myOffset = OBJECT_OFFSETOF(AdaptiveInferredPropertyValueWatchpointBase, m_structureWatchpoint);
index 410a93f..98e3381 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2015 Apple Inc. All rights reserved.
+ * Copyright (C) 2015-2017 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -46,6 +46,7 @@ public:
     virtual ~AdaptiveInferredPropertyValueWatchpointBase() = default;
 
 protected:
+    virtual bool isValid() const;
     virtual void handleFire(const FireDetail&) = 0;
 
 private:
index 43bc7ae..4cc09a6 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2016-2017 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
 
 namespace JSC {
 
+bool FreeList::contains(const void* target) const
+{
+    if (remaining) {
+        const void* start = (payloadEnd - remaining);
+        const void* end = payloadEnd;
+        return (start <= target) && (target < end);
+    }
+
+    FreeCell* candidate = head;
+    while (candidate) {
+        if (candidate == target)
+            return true;
+        candidate = candidate->next;
+    }
+
+    return false;
+}
+
 void FreeList::dump(PrintStream& out) const
 {
     out.print("{head = ", RawPointer(head), ", payloadEnd = ", RawPointer(payloadEnd), ", remaining = ", remaining, ", originalSize = ", originalSize, "}");
index 842caa6..e6a5433 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2016-2017 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -80,7 +80,9 @@ struct FreeList {
     {
         return *this != FreeList();
     }
-    
+
+    bool contains(const void* target) const;
+
     bool allocationWillFail() const { return !head && !remaining; }
     bool allocationWillSucceed() const { return !allocationWillFail(); }
     
index ef5e540..eb8e4cd 100644 (file)
@@ -47,7 +47,9 @@ public:
     
     void zap() { *reinterpret_cast_ptr<uintptr_t**>(this) = 0; }
     bool isZapped() const { return !*reinterpret_cast_ptr<uintptr_t* const*>(this); }
-    
+
+    bool isLive();
+
     bool isLargeAllocation() const;
     CellContainer cellContainer() const;
     MarkedBlock& markedBlock() const;
index c2d909f..bde5e4a 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2016-2017 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
 #include "CellContainer.h"
 #include "HeapCell.h"
 #include "LargeAllocation.h"
-#include "MarkedBlock.h"
+#include "MarkedBlockInlines.h"
 #include "VM.h"
 
 namespace JSC {
 
+ALWAYS_INLINE bool HeapCell::isLive()
+{
+    if (isLargeAllocation())
+        return largeAllocation().isLive();
+    auto& markedBlockHandle = markedBlock().handle();
+    if (markedBlockHandle.isFreeListed())
+        return !markedBlockHandle.isFreeListedCell(this);
+    return markedBlockHandle.isLive(this);
+}
+
 ALWAYS_INLINE bool HeapCell::isLargeAllocation() const
 {
     return LargeAllocation::isLargeAllocation(const_cast<HeapCell*>(this));
index 1c6402b..94f217d 100644 (file)
@@ -156,6 +156,8 @@ public:
     void* tryAllocate(GCDeferralContext* = nullptr);
     Heap* heap() { return m_heap; }
 
+    bool isFreeListedCell(const void* target) const { return m_freeList.contains(target); }
+
     template<typename Functor> void forEachBlock(const Functor&);
     template<typename Functor> void forEachNotEmptyBlock(const Functor&);
     
index 821adf8..9f51092 100644 (file)
@@ -166,6 +166,8 @@ public:
         bool isLive(const HeapCell*);
         bool isLiveCell(const void*);
 
+        bool isFreeListedCell(const void* target) const;
+
         bool isNewlyAllocated(const void*);
         void setNewlyAllocated(const void*);
         void clearNewlyAllocated(const void*);
index 78379f3..efd2ed5 100644 (file)
@@ -117,6 +117,12 @@ inline bool MarkedBlock::Handle::isLiveCell(HeapVersion markingVersion, bool isM
     return isLive(markingVersion, isMarking, static_cast<const HeapCell*>(p));
 }
 
+inline bool MarkedBlock::Handle::isFreeListedCell(const void* target) const
+{
+    ASSERT(isFreeListed());
+    return m_allocator->isFreeListedCell(target);
+}
+
 // The following has to be true for specialization to kick in:
 //
 // sweepMode == SweepToFreeList
index c0583a6..21d6248 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2013 Apple Inc. All rights reserved.
+ * Copyright (C) 2013-2017 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -90,6 +90,7 @@ public:
     ObjectToStringAdaptiveInferredPropertyValueWatchpoint(const ObjectPropertyCondition&, StructureRareData*);
 
 private:
+    bool isValid() const override;
     void handleFire(const FireDetail&) override;
 
     StructureRareData* m_structureRareData;
@@ -212,6 +213,11 @@ ObjectToStringAdaptiveInferredPropertyValueWatchpoint::ObjectToStringAdaptiveInf
 {
 }
 
+bool ObjectToStringAdaptiveInferredPropertyValueWatchpoint::isValid() const
+{
+    return m_structureRareData->isLive();
+}
+
 void ObjectToStringAdaptiveInferredPropertyValueWatchpoint::handleFire(const FireDetail& detail)
 {
     StringPrintStream out;