Changeset 96424 in webkit

Timestamp:

Sep 30, 2011 2:15:04 PM (13 years ago)

Author:

ggaren@apple.com

Message:

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.

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• heap/MarkedSpace.cpp (modified) (3 diffs)
• heap/MarkedSpace.h (modified) (4 diffs)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/JavaScriptCore/heap/MarkedSpace.cpp

@@ -36 +36 @@
     , 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;
 }
@@ -65 +65 @@
     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();
 }
@@ -74 +74 @@
 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();
 }

trunk/Source/JavaScriptCore/heap/MarkedSpace.h

@@ -33 +33 @@
 #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 {
@@ -46 +46 @@
     WTF_MAKE_NONCOPYABLE(MarkedSpace);
 public:
-    static const size_t maxCellSize = 1024;
+    static const size_t maxCellSize = 2048;
 
     struct SizeClass {
@@ -79 +79 @@
 
 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;
@@ -114 +113 @@
 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];