Bug 200989 – Crash under StringImpl::~StringImpl() in IDBServer::computeSpaceUsedForOrigin()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

Crash under StringImpl::~StringImpl() in IDBServer::computeSpaceUsedForOrigin():
Thread[5] EXC_BAD_ACCESS (SIGSEGV) (KERN_INVALID_ADDRESS at 0x6f697463656e6e6f -> 0x00000063656e6e6f (possible pointer authentication failure))
[  0] 0x0000000195c88438 JavaScriptCore`WTF::StringImpl::~StringImpl() [inlined] WTF::StringImpl::deref() at StringImpl.h:1074:29

     0x0000000195c88428:      ldp x29, x30, [sp, #0x10]
     0x0000000195c8842c:      ldp x20, x19, [sp], #0x20
     0x0000000195c88430:    retab 
     0x0000000195c88434:      ldr x0, [x19, #0x18]
 ->  0x0000000195c88438:      ldr w8, [x0]
     0x0000000195c8843c:     subs w8, w8, #0x2         ; =0x2 
     0x0000000195c88440:     b.eq 0x3d458              ; <+272> [inlined] WTF::StringImpl::~StringImpl() at StringImpl.cpp:150
     0x0000000195c88444:      str w8, [x0]
     0x0000000195c88448:      mov x0, x19

[  0] 0x0000000195c88438 JavaScriptCore`WTF::StringImpl::~StringImpl() + 240 at StringImpl.cpp:145
       141 	    }
       142 	
       143 	    ASSERT(ownership == BufferSubstring);
       144 	    ASSERT(substringBuffer());
    -> 145 	    substringBuffer()->deref();
       146 	}
       147 	
       148 	void StringImpl::destroy(StringImpl* stringImpl)
       149 	{
    
[  1] 0x0000000195c8849f JavaScriptCore`WTF::StringImpl::destroy(WTF::StringImpl*) [inlined] WTF::StringImpl::~StringImpl() + 3 at StringImpl.cpp:108:1
       104 	
       105 	StringImpl::StaticStringImpl StringImpl::s_emptyAtomString("", StringImpl::StringAtom);
       106 	
       107 	StringImpl::~StringImpl()
    -> 108 	{
       109 	    ASSERT(!isStatic());
       110 	
       111 	    StringView::invalidate(*this);
       112 	
    
[  1] 0x0000000195c8849c JavaScriptCore`WTF::StringImpl::destroy(WTF::StringImpl*) + 12 at StringImpl.cpp:150
       146 	}
       147 	
       148 	void StringImpl::destroy(StringImpl* stringImpl)
       149 	{
    -> 150 	    stringImpl->~StringImpl();
       151 	    fastFree(stringImpl);
       152 	}
       153 	
       154 	Ref<StringImpl> StringImpl::createFromLiteral(const char* characters, unsigned length)
    
[  2] 0x0000000195c8849f JavaScriptCore`WTF::StringImpl::destroy(WTF::StringImpl*) [inlined] WTF::StringImpl::~StringImpl() + 3 at StringImpl.cpp:108:1
       104 	
       105 	StringImpl::StaticStringImpl StringImpl::s_emptyAtomString("", StringImpl::StringAtom);
       106 	
       107 	StringImpl::~StringImpl()
    -> 108 	{
       109 	    ASSERT(!isStatic());
       110 	
       111 	    StringView::invalidate(*this);
       112 	
    
[  2] 0x0000000195c8849c JavaScriptCore`WTF::StringImpl::destroy(WTF::StringImpl*) + 12 at StringImpl.cpp:150
       146 	}
       147 	
       148 	void StringImpl::destroy(StringImpl* stringImpl)
       149 	{
    -> 150 	    stringImpl->~StringImpl();
       151 	    fastFree(stringImpl);
       152 	}
       153 	
       154 	Ref<StringImpl> StringImpl::createFromLiteral(const char* characters, unsigned length)
    
[  3] 0x0000000195c68703 JavaScriptCore`WTF::FileSystemImpl::pathByAppendingComponent(WTF::String const&, WTF::String const&) + 563 at FileSystemPOSIX.cpp:0
       1   	/*
       2   	 * Copyright (C) 2007-2017 Apple Inc. All rights reserved.
       3   	 *
       4   	 * Redistribution and use in source and binary forms, with or without
       5   	 * modification, are permitted provided that the following conditions
       6   	 * are met:
       7   	 *
       8   	 * 1.  Redistributions of source code must retain the above copyright
       9   	 *     notice, this list of conditions and the following disclaimer.
    
ok
[  4] 0x000000018f3c9b77 WebCore`WebCore::IDBDatabaseIdentifier::databaseDirectoryRelativeToRoot(WebCore::SecurityOriginData const&, WebCore::SecurityOriginData const&, WTF::String const&, WTF::String const&) + 51 at IDBDatabaseIdentifier.cpp:65:31
       61  	}
       62  	
       63  	String IDBDatabaseIdentifier::databaseDirectoryRelativeToRoot(const SecurityOriginData& topLevelOrigin, const SecurityOriginData& openingOrigin, const String& rootDirectory, const String& versionString)
       64  	{
    -> 65  	    String versionDirectory = FileSystem::pathByAppendingComponent(rootDirectory, versionString);
       66  	    String mainFrameDirectory = FileSystem::pathByAppendingComponent(versionDirectory, topLevelOrigin.databaseIdentifier());
       67  	
       68  	    // If the opening origin and main frame origins are the same, there is no partitioning.
       69  	    if (openingOrigin == topLevelOrigin)
    
[  5] 0x000000018f40a687 WebCore`WebCore::IDBServer::IDBServer::computeSpaceUsedForOrigin(WebCore::ClientOrigin const&) + 79 at IDBServer.cpp:778:38
       774 	void IDBServer::computeSpaceUsedForOrigin(const ClientOrigin& origin)
       775 	{
       776 	    ASSERT(!isMainThread());
       777 	
    -> 778 	    auto oldVersionOriginDirectory = IDBDatabaseIdentifier::databaseDirectoryRelativeToRoot(origin.topOrigin, origin.clientOrigin, m_databaseDirectoryPath, "v0");
       779 	    auto newVersionOriginDirectory = IDBDatabaseIdentifier::databaseDirectoryRelativeToRoot(origin.topOrigin, origin.clientOrigin, m_databaseDirectoryPath, "v1");
       780 	    auto size = SQLiteIDBBackingStore::databasesSizeForFolder(oldVersionOriginDirectory) + SQLiteIDBBackingStore::databasesSizeForFolder(newVersionOriginDirectory);
       781 	
       782 	    postDatabaseTaskReply(createCrossThreadTask(*this, &IDBServer::finishComputingSpaceUsedForOrigin, origin, size));
    
[  6] 0x000000018f4172db WebCore`WTF::Detail::CallableWrapper<WTF::CrossThreadTask WTF::createCrossThreadTask<WebCore::IDBServer::IDBServer, 0, WebCore::ClientOrigin const&, WebCore::ClientOrigin>(WebCore::IDBServer::IDBServer&, void (WebCore::IDBServer::IDBServer::*)(WebCore::ClientOrigin const&), WebCore::ClientOrigin const&)::'lambda'(), void>::call() [inlined] void WTF::callMemberFunctionForCrossThreadTaskImpl<WebCore::IDBServer::IDBServer, void (WebCore::IDBServer::IDBServer::*)(WebCore::ClientOrigin const&), std::__1::tuple<WebCore::ClientOrigin>, 0ul>(WebCore::IDBServer::IDBServer*, void (WebCore::IDBServer::IDBServer::*)(WebCore::ClientOrigin const&), std::__1::tuple<WebCore::ClientOrigin>&&, std::__1::integer_sequence<unsigned long, 0ul>) + 63 at CrossThreadTask.h:78:5
[  6] 0x000000018f41729c WebCore`WTF::Detail::CallableWrapper<WTF::CrossThreadTask WTF::createCrossThreadTask<WebCore::IDBServer::IDBServer, 0, WebCore::ClientOrigin const&, WebCore::ClientOrigin>(WebCore::IDBServer::IDBServer&, void (WebCore::IDBServer::IDBServer::*)(WebCore::ClientOrigin const&), WebCore::ClientOrigin const&)::'lambda'(), void>::call() [inlined] void WTF::callMemberFunctionForCrossThreadTask<WebCore::IDBServer::IDBServer, void (WebCore::IDBServer::IDBServer::*)(WebCore::ClientOrigin const&), std::__1::tuple<WebCore::ClientOrigin>, std::__1::integer_sequence<unsigned long, 0ul> >(WebCore::IDBServer::IDBServer*, void (WebCore::IDBServer::IDBServer::*)(WebCore::ClientOrigin const&), std::__1::tuple<WebCore::ClientOrigin>&&) at CrossThreadTask.h:84
[  6] 0x000000018f41729c WebCore`WTF::Detail::CallableWrapper<WTF::CrossThreadTask WTF::createCrossThreadTask<WebCore::IDBServer::IDBServer, 0, WebCore::ClientOrigin const&, WebCore::ClientOrigin>(WebCore::IDBServer::IDBServer&, void (WebCore::IDBServer::IDBServer::*)(WebCore::ClientOrigin const&), WebCore::ClientOrigin const&)::'lambda'(), void>::call() [inlined] WTF::CrossThreadTask WTF::createCrossThreadTask<WebCore::IDBServer::IDBServer, 0, WebCore::ClientOrigin const&, WebCore::ClientOrigin>(WebCore::IDBServer::IDBServer&, void (WebCore::IDBServer::IDBServer::*)(WebCore::ClientOrigin const&), WebCore::ClientOrigin const&)::'lambda'()::operator()() + 8 at CrossThreadTask.h:99
[  6] 0x000000018f417294 WebCore`WTF::Detail::CallableWrapper<WTF::CrossThreadTask WTF::createCrossThreadTask<WebCore::IDBServer::IDBServer, 0, WebCore::ClientOrigin const&, WebCore::ClientOrigin>(WebCore::IDBServer::IDBServer&, void (WebCore::IDBServer::IDBServer::*)(WebCore::ClientOrigin const&), WebCore::ClientOrigin const&)::'lambda'(), void>::call() at Function.h:52
[  7] 0x0000000195c5d9a3 JavaScriptCore`WTF::CrossThreadTaskHandler::taskRunLoop() [inlined] WTF::Function<void ()>::operator()() const + 19 at Function.h:79:35
[  7] 0x0000000195c5d990 JavaScriptCore`WTF::CrossThreadTaskHandler::taskRunLoop() [inlined] WTF::CrossThreadTask::performTask() at CrossThreadTask.h:48
[  7] 0x0000000195c5d990 JavaScriptCore`WTF::CrossThreadTaskHandler::taskRunLoop() + 236 at CrossThreadTaskHandler.cpp:78

[Radar WebKit Bug Importer]

<rdar://problem/54565546>

[Chris Dumez]

Created attachment 376901 [details]
Patch

[Alex Christensen]

Comment on attachment 376901 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/server/IDBServer.h:133
> +    String databaseDirectoryPath() const { return m_databaseDirectoryPath.isolatedCopy(); }

There's nothing obvious about this code location that indicates it's going to be sent to another thread.

[Chris Dumez]

(In reply to Alex Christensen from comment #3)
> Comment on attachment 376901 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=376901&action=review
> 
> > Source/WebCore/Modules/indexeddb/server/IDBServer.h:133
> > +    String databaseDirectoryPath() const { return m_databaseDirectoryPath.isolatedCopy(); }
> 
> There's nothing obvious about this code location that indicates it's going
> to be sent to another thread.

What's the concern here? Performance? I doubt this is hot. Also, this is private and mostly called from background threads.

[Alex Christensen]

My concern is, "are there other strings that are being passed to other threads with this string?" is a hard question to answer by looking at this change.

[Chris Dumez]

(In reply to Chris Dumez from comment #4)
> (In reply to Alex Christensen from comment #3)
> > Comment on attachment 376901 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=376901&action=review
> > 
> > > Source/WebCore/Modules/indexeddb/server/IDBServer.h:133
> > > +    String databaseDirectoryPath() const { return m_databaseDirectoryPath.isolatedCopy(); }
> > 
> > There's nothing obvious about this code location that indicates it's going
> > to be sent to another thread.
> 
> What's the concern here? Performance? I doubt this is hot. Also, this is
> private and mostly called from background threads.

Also, we already use this pattern in network cache code at least:
String Storage::basePath() const
{
    return m_basePath.isolatedCopy();
}

String Storage::versionPath() const
{
    return makeVersionedDirectoryPath(basePath());
}

String Storage::recordsPath() const
{
    return m_recordsPath.isolatedCopy();
}

[Chris Dumez]

(In reply to Alex Christensen from comment #5)
> My concern is, "are there other strings that are being passed to other
> threads with this string?" is a hard question to answer by looking at this
> change.

You can see in the bug description the crash trace:
auto oldVersionOriginDirectory = IDBDatabaseIdentifier::databaseDirectoryRelativeToRoot(origin.topOrigin, origin.clientOrigin, m_databaseDirectoryPath, "v0");

and then:
String versionDirectory = FileSystem::pathByAppendingComponent(rootDirectory, versionString);

So issue is either with m_databaseDirectoryPath or "v0". It cannot be "v0" so it must be m_databaseDirectoryPath. origin is properly isolated copied:
postDatabaseTask(createCrossThreadTask(*this, &IDBServer::computeSpaceUsedForOrigin, origin));

[WebKit Commit Bot]

Comment on attachment 376901 [details]
Patch

Clearing flags on attachment: 376901

Committed r248969: <https://trac.webkit.org/changeset/248969>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.