Bug 207324 – KeyedDecoderGeneric fails to allocate Vector while decoding broken data

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Fujii Hironori]

KeyedDecoderGeneric fails to allocate Vector while decoding broken data

AppleWin WK1 and WinCairo WK1 are sharing same data directory even though they are using different KeyedEncoder/KeyedDecoder format.

1. Start AppleWin WK1 MiniBrowser, Open Web Inspector, Change Setting, for example, zoom scale, Close the AppleWin WK1
2. Start WinCairo WK1 MiniBrowser, Open Web Inspector
3. Crash

Callstack:

> WTF.dll!WTFCrash() Line 305	C++
> WebKit.dll!WTF::VectorBufferBase<unsigned char,WTF::FastMalloc>::allocateBuffer(unsigned __int64 newCapacity=12884901888) Line 290	C++
> WebKit.dll!WTF::VectorBuffer<unsigned char,0,WTF::FastMalloc>::VectorBuffer<unsigned char,0,WTF::FastMalloc>(unsigned __int64 capacity=12884901888, unsigned __int64 size=12884901888) Line 394	C++
> WebKit.dll!WTF::Vector<unsigned char,0,WTF::CrashOnOverflow,16,WTF::FastMalloc>::Vector<unsigned char,0,WTF::CrashOnOverflow,16,WTF::FastMalloc>(unsigned __int64 size=12884901888) Line 630	C++
> WebKit.dll!WebCore::readString(WTF::Persistence::Decoder & decoder={...}, WTF::String & result={...}) Line 62	C++
> WebKit.dll!WebCore::KeyedDecoderGeneric::KeyedDecoderGeneric(const unsigned char * data=0x0000021665b51690, unsigned __int64 size=53) Line 104	C++
> [External Code]	
> WebKit.dll!WTF::makeUnique<WebCore::KeyedDecoderGeneric,unsigned char const * &,unsigned __int64 &>(const unsigned char * & <args_0>=0x0000021665b51690, unsigned __int64 & <args_1>=53) Line 483	C++
> WebKit.dll!WebCore::KeyedDecoder::decoder(const unsigned char * data=0x0000021665b51690, unsigned __int64 size=53) Line 88	C++
> WebKit.dll!WebCore::deserializeIDBKeyPath(const unsigned char * data=0x0000021665b51690, unsigned __int64 size=53, WTF::Optional<WTF::Variant<WTF::String,WTF::Vector<WTF::String,0,WTF::CrashOnOverflow,16,WTF::FastMalloc>>> & result={...}) Line 72	C++
> WebKit.dll!WebCore::IDBServer::SQLiteIDBBackingStore::extractExistingDatabaseInfo() Line 767	C++
> WebKit.dll!WebCore::IDBServer::SQLiteIDBBackingStore::getOrEstablishDatabaseInfo(WebCore::IDBDatabaseInfo & info={...}) Line 994	C++
> WebKit.dll!WebCore::IDBServer::UniqueIDBDatabase::performCurrentOpenOperation() Line 176	C++
> WebKit.dll!WebCore::IDBServer::UniqueIDBDatabase::handleCurrentOperation() Line 357	C++
> WebKit.dll!WebCore::IDBServer::UniqueIDBDatabase::handleDatabaseOperations() Line 340	C++
> WebKit.dll!WebCore::IDBServer::UniqueIDBDatabase::openDatabaseConnection(WebCore::IDBServer::IDBConnectionToClient & connection={...}, const WebCore::IDBRequestData & requestData={...}) Line 153	C++
> WebKit.dll!WebCore::IDBServer::IDBServer::openDatabase(const WebCore::IDBRequestData & requestData={...}) Line 152	C++
> WebKit.dll!`InProcessIDBServer::openDatabase'::`2'::<lambda_1>::operator()() Line 145	C++
> WebKit.dll!WTF::Detail::CallableWrapper<`InProcessIDBServer::openDatabase'::`2'::<lambda_1>,void>::call() Line 52	C++
> WebKit.dll!WTF::Function<void __cdecl(void)>::operator()() Line 85	C++
> WebKit.dll!WebCore::StorageThread::threadEntryPoint() Line 79	C++
> WebKit.dll!`WebCore::StorageThread::start'::`17'::<lambda_2>::operator()() Line 66	C++
> WebKit.dll!WTF::Detail::CallableWrapper<`WebCore::StorageThread::start'::`17'::<lambda_2>,void>::call() Line 52	C++
> WTF.dll!WTF::Function<void __cdecl(void)>::operator()() Line 85	C++
> WTF.dll!WTF::Thread::entryPoint(WTF::Thread::NewThreadContext * newThreadContext=0x000002164eea6c70) Line 149	C++
> WTF.dll!WTF::wtfThreadEntryPoint(void * data=0x000002164eea6c70) Line 153	C++
> [External Code]	

In above case, trying to allocate Vector with size=12884901888.

[Fujii Hironori]

Created attachment 393626 [details]
Patch

[Darin Adler]

Comment on attachment 393626 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393626&action=review

> Tools/ChangeLog:10
> +        (TestWebKitAPI::KeyedCoding.DecodeRandomData): Added a new test decoding random data.

I think a better test is decoding pseudo-random data with a fixed seed. We don’t want a test that randomly fails.

[Fujii Hironori]

Comment on attachment 393626 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393626&action=review

>> Tools/ChangeLog:10
>> +        (TestWebKitAPI::KeyedCoding.DecodeRandomData): Added a new test decoding random data.
> 
> I think a better test is decoding pseudo-random data with a fixed seed. We don’t want a test that randomly fails.

Agreed. Will do so.

[Fujii Hironori]

I found two more crash bugs in KeyedDecoderGeneric by changing the random seed.

[Darin Adler]

Comment on attachment 393626 [details]
Patch

I see the same mistake in:

1) decodeCFData in CertificateInfo.h
2) AuthenticatorResponseData::decode where it also uses ArrayBuffer::create but should be using ArrayBuffer::tryCreate
3) SerializedScriptValue::decode
4) decodeSharedBuffer and decodeTypesAndData in WebCoreArgumentCoders.cpp

We need someone to fix all of those. May not be as easy to write tests for those.

[Fujii Hironori]

OK, I will try to fix them.

[Fujii Hironori]

Created attachment 393629 [details]
Patch

[Fujii Hironori]

Comment on attachment 393629 [details]
Patch

Clearing flags on attachment: 393629

Committed r258486: <https://trac.webkit.org/changeset/258486>

[Fujii Hironori]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/60479609>

[Fujii Hironori]

(In reply to Darin Adler from comment #5)
> 
> I see the same mistake in:
> 
> 1) decodeCFData in CertificateInfo.h
> 2) AuthenticatorResponseData::decode where it also uses ArrayBuffer::create
> but should be using ArrayBuffer::tryCreate
> 3) SerializedScriptValue::decode
> 4) decodeSharedBuffer and decodeTypesAndData in WebCoreArgumentCoders.cpp
> 
> We need someone to fix all of those. May not be as easy to write tests for
> those.

Filed: Bug 209131 – Don't allocate a buffer with the decoded size without ensuring bufferIsLargeEnoughToContain(size)