Bug 69148 – Crash due to out of bounds read/write in MarkedSpace

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

With JSC from http://trac.webkit.org/browser/releases/WebKitGTK/webkit-1.4.2 on Windows and Mac.

MarkedSpace can crash as MarkedSpace::sizeClassFor() accesses m_impreciseSizeClasses out of bounds.

* Call MarkedSpace::sizeClassFor(952), e.g. from Heap::allocate(952);
* Notice this attempts to access m_impreciseSizeClasses[7] but the size of the array is only 7.
* This crashes later as it uses the address of the next member, HashSet<MarkedBlock*> m_blocks, as the SizeClass and trashes it.

Also if you pass 121-127 in to sizeClassFor() it access the out of bounds m_preciseSizeClasses[15] element but due to the class layout this is m_impreciseSizeClasses[0] so it kind of works.

I thought the bug was an out by one error in the size of both 
MarkedSpace::m_preciseSizeClasses and MarkedSpace::m_impreciseSizeClasses i.e. they should be 

static const size_t preciseCount = preciseCutoff / preciseStep;
static const size_t impreciseCount = impreciseCutoff / impreciseStep;

but the way its used in MarkedSpace::MarkedSpace() and MarkedSpace::reset() seem inconsistent with that.

I'm not clear on whether SizeClass.cellSize was meant to be an upper bound or lower bound.



Callstack:

>	jscd.dll!WTF::HashTable<WTF::StringImpl *,WTF::StringImpl *,WTF::IdentityExtractor<WTF::StringImpl *>,WTF::StringHash,WTF::HashTraits<WTF::StringImpl *>,WTF::HashTraits<WTF::StringImpl *> >::isEmptyBucket(WTF::StringImpl * const & value=)  Line 339 + 0x19 bytes	C++
 	jscd.dll!WTF::HashTable<char const *,std::pair<char const *,WTF::RefPtr<WTF::StringImpl> >,WTF::PairFirstExtractor<std::pair<char const *,WTF::RefPtr<WTF::StringImpl> > >,WTF::PtrHash<char const *>,WTF::PairHashTraits<WTF::HashTraits<char const *>,WTF::HashTraits<WTF::RefPtr<WTF::StringImpl> > >,WTF::HashTraits<char const *> >::isEmptyOrDeletedBucket(const std::pair<char const *,WTF::RefPtr<WTF::StringImpl> > & value=(...,{m_ptr=??? }))  Line 341 + 0xd bytes	C++
 	jscd.dll!WTF::HashTableConstIterator<WTF::StringImpl *,WTF::StringImpl *,WTF::IdentityExtractor<WTF::StringImpl *>,WTF::StringHash,WTF::HashTraits<WTF::StringImpl *>,WTF::HashTraits<WTF::StringImpl *> >::skipEmptyBuckets()  Line 109 + 0x18 bytes	C++
 	jscd.dll!WTF::HashTableConstIterator<WTF::StringImpl *,WTF::StringImpl *,WTF::IdentityExtractor<WTF::StringImpl *>,WTF::StringHash,WTF::HashTraits<WTF::StringImpl *>,WTF::HashTraits<WTF::StringImpl *> >::HashTableConstIterator<WTF::StringImpl *,WTF::StringImpl *,WTF::IdentityExtractor<WTF::StringImpl *>,WTF::StringHash,WTF::HashTraits<WTF::StringImpl *>,WTF::HashTraits<WTF::StringImpl *> >(const WTF::HashTable<WTF::StringImpl *,WTF::StringImpl *,WTF::IdentityExtractor<WTF::StringImpl *>,WTF::StringHash,WTF::HashTraits<WTF::StringImpl *>,WTF::HashTraits<WTF::StringImpl *> > * table=0x0d1eaff4, WTF::StringImpl * const * position=0x00000000, WTF::StringImpl * const * endPosition=0x00000100)  Line 118	C++
 	jscd.dll!WTF::HashTable<WTF::StringImpl *,WTF::StringImpl *,WTF::IdentityExtractor<WTF::StringImpl *>,WTF::StringHash,WTF::HashTraits<WTF::StringImpl *>,WTF::HashTraits<WTF::StringImpl *> >::makeConstIterator(WTF::StringImpl * * pos=0x00000000)  Line 392 + 0x26 bytes	C++
 	jscd.dll!WTF::HashTable<WTF::StringImpl *,WTF::StringImpl *,WTF::IdentityExtractor<WTF::StringImpl *>,WTF::StringHash,WTF::HashTraits<WTF::StringImpl *>,WTF::HashTraits<WTF::StringImpl *> >::begin()  Line 310 + 0x19 bytes	C++
 	jscd.dll!WTF::HashSet<JSC::MarkedBlock *,WTF::PtrHash<JSC::MarkedBlock *>,WTF::HashTraits<JSC::MarkedBlock *> >::begin()  Line 139 + 0xc bytes	C++
 	jscd.dll!JSC::MarkedSpace::clearMarks()  Line 113 + 0x12 bytes	C++
 	jscd.dll!JSC::Heap::markRoots()  Line 227	C++
 	jscd.dll!JSC::Heap::reset(JSC::Heap::SweepToggle sweepToggle=DoNotSweep)  Line 378	C++
 	jscd.dll!JSC::Heap::allocateSlowCase(unsigned int bytes=952)  Line 126	C++
 	npturbulenz.dll!JSC::Heap::allocate(unsigned int bytes=952)  Line 420	C++
 	npturbulenz.dll!JSC::JSCell::operator new(unsigned int size=952, JSC::JSGlobalData * globalData=0x0d1e9fd8)  Line 425	C++

[Geoffrey Garen]

> I'm not clear on whether SizeClass.cellSize was meant to be an upper bound or lower bound.

Upper bound.

[Geoffrey Garen]

The bug here is that 952 is bigger than the biggest object the Heap can allocate. Another bug is that the Heap's object size assertions did not kick in, since they assume that the Heap can allocate up to maxCellSize, when in reality it can only allocate up to maxCellSize - impreciseStep.

[Geoffrey Garen]

Created attachment 109333 [details]
Patch

[Geoffrey Garen]

Committed r96424: <http://trac.webkit.org/changeset/96424>

[Adam Roben (:aroben)]

Is it possible to write a test for this?

[Geoffrey Garen]

(In reply to comment #5)
> Is it possible to write a test for this?

It's not currently possible because there are no subclasses of JSCell that are 952 bytes. I'm not sure how WebKitGTK ran into this issue. (Maybe I'm forgetting about a particular subclass, though?)

[Adam Roben (:aroben)]

(In reply to comment #6)
> (In reply to comment #5)
> > Is it possible to write a test for this?
> 
> It's not currently possible because there are no subclasses of JSCell that are 952 bytes. I'm not sure how WebKitGTK ran into this issue. (Maybe I'm forgetting about a particular subclass, though?)

Thanks for the explanation. In the future it would be nice to put information like this in the ChangeLog.