Bug 200989

Summary: Crash under StringImpl::~StringImpl() in IDBServer::computeSpaceUsedForOrigin()
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit2Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, alecflett, beidson, commit-queue, ews-watchlist, ggaren, jsbell, sihui_liu, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=200990
Attachments:
Description Flags
Patch none

Chris Dumez
Reported 2019-08-21 11:55:13 PDT
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
Attachments
Patch (9.00 KB, patch)
2019-08-21 12:09 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2019-08-21 11:55:32 PDT
<rdar://problem/54565546>
Chris Dumez
Comment 2 2019-08-21 12:09:27 PDT
Created attachment 376901 [details] Patch
Alex Christensen
Comment 3 2019-08-21 12:27:14 PDT
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
Comment 4 2019-08-21 12:28:44 PDT
(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
Comment 5 2019-08-21 12:29:45 PDT
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
Comment 6 2019-08-21 12:30:14 PDT
(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
Comment 7 2019-08-21 12:32:11 PDT
(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 8 2019-08-21 15:32:29 PDT
Comment on attachment 376901 [details] Patch Clearing flags on attachment: 376901 Committed r248969: <https://trac.webkit.org/changeset/248969>
WebKit Commit Bot
Comment 9 2019-08-21 15:32:31 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.