Crash due to out of bounds read/write in MarkedSpace

https://bugs.webkit.org/show_bug.cgi?id=69148

This was a case of being surprised by a poorly aritulcated cell size limit,
plus an incorrect ASSERT guarding the cell size limit.

Reviewed by Oliver Hunt.

* heap/MarkedSpace.h:
(JSC::MarkedSpace::sizeClassFor): Changed heap size ranges to be inclusive,
since it makes the ranges easier to understand.

Bumped up the max cell size to support the use case in this bug. Since the
atomSize is much bigger than it used to be, there isn't much accounting
cost to handling more size classes.

Switched to FixedArray, to help catch SizeClass indexing bugs in the future.

* heap/MarkedSpace.cpp:
(JSC::MarkedSpace::MarkedSpace):
(JSC::MarkedSpace::resetAllocator):
(JSC::MarkedSpace::canonicalizeCellLivenessData): Updated for size ranges
being inclusive.

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

diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
index 121b309..c9a1002 100644 (file)
--- a/Source/JavaScriptCore/ChangeLog
+++ b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,29 @@
+2011-09-30  Geoffrey Garen  <ggaren@apple.com>
+
+        Crash due to out of bounds read/write in MarkedSpace
+        https://bugs.webkit.org/show_bug.cgi?id=69148
+        
+        This was a case of being surprised by a poorly aritulcated cell size limit,
+        plus an incorrect ASSERT guarding the cell size limit.
+
+        Reviewed by Oliver Hunt.
+
+        * heap/MarkedSpace.h:
+        (JSC::MarkedSpace::sizeClassFor): Changed heap size ranges to be inclusive,
+        since it makes the ranges easier to understand.
+        
+        Bumped up the max cell size to support the use case in this bug. Since the
+        atomSize is much bigger than it used to be, there isn't much accounting
+        cost to handling more size classes.
+        
+        Switched to FixedArray, to help catch SizeClass indexing bugs in the future.
+
+        * heap/MarkedSpace.cpp:
+        (JSC::MarkedSpace::MarkedSpace):
+        (JSC::MarkedSpace::resetAllocator):
+        (JSC::MarkedSpace::canonicalizeCellLivenessData): Updated for size ranges
+        being inclusive.
+
 2011-09-30  Pierre Rossi  <pierre.rossi@gmail.com>
 
         [Qt] Build fix: Qt::escape is deprecated in Qt5

diff --git a/Source/JavaScriptCore/heap/MarkedSpace.cpp b/Source/JavaScriptCore/heap/MarkedSpace.cpp
index 34cbe75..04cb937 100644 (file)
--- a/Source/JavaScriptCore/heap/MarkedSpace.cpp
+++ b/Source/JavaScriptCore/heap/MarkedSpace.cpp
@@ -35,10 +35,10 @@ MarkedSpace::MarkedSpace(Heap* heap)
     , m_highWaterMark(0)
     , m_heap(heap)
 {
-    for (size_t cellSize = preciseStep; cellSize < preciseCutoff; cellSize += preciseStep)
+    for (size_t cellSize = preciseStep; cellSize <= preciseCutoff; cellSize += preciseStep)
         sizeClassFor(cellSize).cellSize = cellSize;
 
-    for (size_t cellSize = impreciseStep; cellSize < impreciseCutoff; cellSize += impreciseStep)
+    for (size_t cellSize = impreciseStep; cellSize <= impreciseCutoff; cellSize += impreciseStep)
         sizeClassFor(cellSize).cellSize = cellSize;
 }
 
@@ -64,19 +64,19 @@ void MarkedSpace::resetAllocator()
 {
     m_waterMark = 0;
 
-    for (size_t cellSize = preciseStep; cellSize < preciseCutoff; cellSize += preciseStep)
+    for (size_t cellSize = preciseStep; cellSize <= preciseCutoff; cellSize += preciseStep)
         sizeClassFor(cellSize).resetAllocator();
 
-    for (size_t cellSize = impreciseStep; cellSize < impreciseCutoff; cellSize += impreciseStep)
+    for (size_t cellSize = impreciseStep; cellSize <= impreciseCutoff; cellSize += impreciseStep)
         sizeClassFor(cellSize).resetAllocator();
 }
 
 void MarkedSpace::canonicalizeCellLivenessData()
 {
-    for (size_t cellSize = preciseStep; cellSize < preciseCutoff; cellSize += preciseStep)
+    for (size_t cellSize = preciseStep; cellSize <= preciseCutoff; cellSize += preciseStep)
         sizeClassFor(cellSize).zapFreeList();
 
-    for (size_t cellSize = impreciseStep; cellSize < impreciseCutoff; cellSize += impreciseStep)
+    for (size_t cellSize = impreciseStep; cellSize <= impreciseCutoff; cellSize += impreciseStep)
         sizeClassFor(cellSize).zapFreeList();
 }
 

diff --git a/Source/JavaScriptCore/heap/MarkedSpace.h b/Source/JavaScriptCore/heap/MarkedSpace.h
index 9ec1ac5..1a4d919 100644 (file)
--- a/Source/JavaScriptCore/heap/MarkedSpace.h
+++ b/Source/JavaScriptCore/heap/MarkedSpace.h
@@ -32,7 +32,7 @@
 #include <wtf/Noncopyable.h>
 #include <wtf/Vector.h>
 
-#define ASSERT_CLASS_FITS_IN_CELL(class) COMPILE_ASSERT(sizeof(class) < MarkedSpace::maxCellSize, class_fits_in_cell)
+#define ASSERT_CLASS_FITS_IN_CELL(class) COMPILE_ASSERT(sizeof(class) <= MarkedSpace::maxCellSize, class_fits_in_cell)
 
 namespace JSC {
 
@@ -45,7 +45,7 @@ class SlotVisitor;
 class MarkedSpace {
     WTF_MAKE_NONCOPYABLE(MarkedSpace);
 public:
-    static const size_t maxCellSize = 1024;
+    static const size_t maxCellSize = 2048;
 
     struct SizeClass {
         SizeClass();
@@ -78,19 +78,18 @@ public:
     template<typename Functor> typename Functor::ReturnType forEachBlock();
 
 private:
-    // [ 8, 16... 128 )
+    // [ 32... 256 ]
     static const size_t preciseStep = MarkedBlock::atomSize;
-    static const size_t preciseCutoff = 128;
-    static const size_t maximumPreciseAllocationSize = preciseCutoff - preciseStep;
-    static const size_t preciseCount = preciseCutoff / preciseStep - 1;
+    static const size_t preciseCutoff = 256;
+    static const size_t preciseCount = preciseCutoff / preciseStep;
 
-    // [ 128, 256... 1024 )
+    // [ 512... 2048 ]
     static const size_t impreciseStep = preciseCutoff;
     static const size_t impreciseCutoff = maxCellSize;
-    static const size_t impreciseCount = impreciseCutoff / impreciseStep - 1;
+    static const size_t impreciseCount = impreciseCutoff / impreciseStep;
 
-    SizeClass m_preciseSizeClasses[preciseCount];
-    SizeClass m_impreciseSizeClasses[impreciseCount];
+    FixedArray<SizeClass, preciseCount> m_preciseSizeClasses;
+    FixedArray<SizeClass, impreciseCount> m_impreciseSizeClasses;
     size_t m_waterMark;
     size_t m_highWaterMark;
     Heap* m_heap;
@@ -113,8 +112,8 @@ inline void MarkedSpace::setHighWaterMark(size_t highWaterMark)
 
 inline MarkedSpace::SizeClass& MarkedSpace::sizeClassFor(size_t bytes)
 {
-    ASSERT(bytes && bytes < maxCellSize);
-    if (bytes <= maximumPreciseAllocationSize)
+    ASSERT(bytes && bytes <= maxCellSize);
+    if (bytes <= preciseCutoff)
         return m_preciseSizeClasses[(bytes - 1) / preciseStep];
     return m_impreciseSizeClasses[(bytes - 1) / impreciseStep];
 }