| Summary: | Heap corruption is detected when destructing JSGlobalObject | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Fujii Hironori <Hironori.Fujii> | ||||||||||
| Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | achristensen, benjamin, bfulgham, cdumez, cmarcelo, commit-queue, ddkilzer, ggaren, joepeck, keith_miller, mark.lam, msaboff, saam, webkit-bug-importer, ysuzuki | ||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||
| Version: | WebKit Nightly Build | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Fujii Hironori
2016-04-20 20:03:22 PDT
This is caused by the heaps mismatch of allocating and deallocating.
Allocating in the heap of WebKit.dll, but Deallocating in JavaScriptCore.dll.
JSGlobalObject::createRareDataIfNeeded is inlined,
but JSGlobalObject::~JSGlobalObject is not inlined.
Callstack of allocation:
> WebKit.dll!WTF::Lock::Lock() Line 119 C++
> [External Code]
> WebKit.dll!WTF::HashTable<WTF::RefPtr<OpaqueJSWeakObjectMap>,WTF::RefPtr<OpaqueJSWeakObjectMap>,WTF::IdentityExtractor,WTF::PtrHash<WTF::RefPtr<OpaqueJSWeakObjectMap> >,WTF::HashTraits<WTF::RefPtr<OpaqueJSWeakObjectMap> >,WTF::HashTraits<WTF::RefPtr<OpaqueJSWeakObjectMap> > >::HashTable<WTF::RefPtr<OpaqueJSWeakObjectMap>,WTF::RefPtr<OpaqueJSWeakObjectMap>,WTF::IdentityExtractor,WTF::PtrHash<WTF::RefPtr<OpaqueJSWeakObjectMap> >,WTF::HashTraits<WTF::RefPtr<OpaqueJSWeakObjectMap> >,WTF::HashTraits<WTF::RefPtr<OpaqueJSWeakObjectMap> > >() Line 557 C++
> WebKit.dll!WTF::HashSet<WTF::RefPtr<OpaqueJSWeakObjectMap>,WTF::PtrHash<WTF::RefPtr<OpaqueJSWeakObjectMap> >,WTF::HashTraits<WTF::RefPtr<OpaqueJSWeakObjectMap> > >::HashSet<WTF::RefPtr<OpaqueJSWeakObjectMap>,WTF::PtrHash<WTF::RefPtr<OpaqueJSWeakObjectMap> >,WTF::HashTraits<WTF::RefPtr<OpaqueJSWeakObjectMap> > >() Line 56 C++
> WebKit.dll!JSC::JSGlobalObject::JSGlobalObjectRareData::JSGlobalObjectRareData() Line 188 C++
> [External Code]
> WebKit.dll!JSC::JSGlobalObject::createRareDataIfNeeded() Line 354 C++
> WebKit.dll!JSC::JSGlobalObject::setProfileGroup(unsigned int value) Line 626 C++
> WebKit.dll!WebCore::ScriptController::initScript(WebCore::DOMWrapperWorld & world) Line 262 C++
> WebKit.dll!WebCore::ScriptController::windowShell(WebCore::DOMWrapperWorld & world) Line 90 C++
> WebKit.dll!WebCore::ScriptController::evaluateInWorld(const WebCore::ScriptSourceCode & sourceCode, WebCore::DOMWrapperWorld & world, WebCore::ExceptionDetails * exceptionDetails) Line 154 C++
> WebKit.dll!WebCore::ScriptController::evaluate(const WebCore::ScriptSourceCode & sourceCode, WebCore::ExceptionDetails * exceptionDetails) Line 180 C++
> WebKit.dll!WebCore::ScriptElement::executeScript(const WebCore::ScriptSourceCode & sourceCode) Line 321 C++
> WebKit.dll!WebCore::ScriptElement::prepareScript(const WTF::TextPosition & scriptStartPosition, WebCore::ScriptElement::LegacyTypeSupport supportLegacyTypes) Line 245 C++
> WebKit.dll!WebCore::HTMLScriptRunner::runScript(WebCore::Element * script, const WTF::TextPosition & scriptStartPosition) Line 304 C++
> WebKit.dll!WebCore::HTMLScriptRunner::execute(WTF::PassRefPtr<WebCore::Element> scriptElement, const WTF::TextPosition & scriptStartPosition) Line 177 C++
> WebKit.dll!WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder() Line 215 C++
> WebKit.dll!WebCore::HTMLDocumentParser::pumpTokenizerLoop(WebCore::HTMLDocumentParser::SynchronousMode mode, bool parsingFragment, WebCore::PumpSession & session) Line 234 C++
> WebKit.dll!WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode mode) Line 282 C++
> WebKit.dll!WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode mode) Line 169 C++
> WebKit.dll!WebCore::HTMLDocumentParser::append(WTF::RefPtr<WTF::StringImpl> && inputSource) Line 396 C++
> WebKit.dll!WebCore::DecodedDataDocumentParser::flush(WebCore::DocumentWriter & writer) Line 60 C++
> WebKit.dll!WebCore::DocumentWriter::end() Line 255 C++
> WebKit.dll!WebCore::DocumentLoader::finishedLoading(double finishTime) Line 437 C++
> WebKit.dll!WebCore::DocumentLoader::notifyFinished(WebCore::CachedResource * resource) Line 384 C++
> WebKit.dll!WebCore::CachedResource::checkNotify() Line 299 C++
> WebKit.dll!WebCore::CachedResource::finishLoading(WebCore::SharedBuffer * __formal) Line 316 C++
> WebKit.dll!WebCore::CachedRawResource::finishLoading(WebCore::SharedBuffer * data) Line 104 C++
> WebKit.dll!WebCore::SubresourceLoader::didFinishLoading(double finishTime) Line 431 C++
> WebKit.dll!WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle * __formal, double finishTime) Line 643 C++
> WebKit.dll!WebCore::ResourceHandleManager::downloadTimerCallback() Line 700 C++
> [External Code]
> WebKit.dll!WebCore::Timer::fired() Line 142 C++
> WebKit.dll!WebCore::ThreadTimers::sharedTimerFiredInternal() Line 124 C++
> WebKit.dll!WebCore::ThreadTimers::setSharedTimer::__l8::<lambda>() Line 73 C++
> [External Code]
> WebKit.dll!WebCore::MainThreadSharedTimer::fired() Line 53 C++
> WebKit.dll!WebCore::TimerWindowWndProc(HWND__ * hWnd, unsigned int message, unsigned __int64 wParam, __int64 lParam) Line 91 C++
> [External Code]
> DumpRenderTreeLib.dll!runTest(const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & inputLine) Line 1145 C++
> DumpRenderTreeLib.dll!main(int argc, const char * * argv) Line 1486 C++
> DumpRenderTreeLib.dll!dllLauncherEntryPoint(int argc, const char * * argv) Line 1517 C++
> DumpRenderTree.exe!main(int argc, const char * * argv) Line 260 C++
> [External Code]
There are two ways to solve this problem: 1) Not inlined JSGlobalObject::createRareDataIfNeeded or part of it 2) Apply WTF_MAKE_FAST_ALLOCATED the classes allocated from inline functions I don't know why AppleWin port does not have this problem, but WinCairo port does. Created attachment 276896 [details]
Patch
(In reply to comment #2) > This is caused by the heaps mismatch of allocating and deallocating. > Allocating in the heap of WebKit.dll, but Deallocating in JavaScriptCore.dll. > > JSGlobalObject::createRareDataIfNeeded is inlined, > but JSGlobalObject::~JSGlobalObject is not inlined. Why is this an issue? Shouldn't both WebKit.dll and JavaScripCore.dll be allocating/deallocating from the same heap of the process that loaded them? Can you please elaborate? Thank you for reviewing my patch. (In reply to comment #4) > Why is this an issue? Shouldn't both WebKit.dll and JavaScripCore.dll be > allocating/deallocating from the same heap of the process that loaded them? WebKit uses CRT static libarary. In Source/cmake/OptionsWin.cmake: > # Use the multithreaded static runtime library instead of the default DLL runtime. > string(REGEX REPLACE "/MD" "/MT" ${flag_var} "${${flag_var}}") Then, Potential Errors Passing CRT Objects Across DLL Boundaries https://msdn.microsoft.com/en-US/library/ms235460(v=vs.110).aspx > Also, because each copy of the CRT library has its own heap > manager, allocating memory in one CRT library and passing the > pointer across a DLL boundary to be freed by a different copy of > the CRT library is a potential cause for heap corruption. Comment on attachment 276896 [details] Patch (In reply to comment #5) > Thank you for reviewing my patch. > > (In reply to comment #4) > > Why is this an issue? Shouldn't both WebKit.dll and JavaScripCore.dll be > > allocating/deallocating from the same heap of the process that loaded them? > > WebKit uses CRT static libarary. > In Source/cmake/OptionsWin.cmake: > > > # Use the multithreaded static runtime library instead of the default DLL runtime. > > string(REGEX REPLACE "/MD" "/MT" ${flag_var} "${${flag_var}}") > > Then, > > Potential Errors Passing CRT Objects Across DLL Boundaries > https://msdn.microsoft.com/en-US/library/ms235460(v=vs.110).aspx > > > Also, because each copy of the CRT library has its own heap > > manager, allocating memory in one CRT library and passing the > > pointer across a DLL boundary to be freed by a different copy of > > the CRT library is a potential cause for heap corruption. This is the kind of good information that we should have in the ChangeLog to justify the change. Please add it. The fix to add WTF_MAKE_FAST_ALLOCATED looks good to me. I'll re-review after you've updated the patch. r- for now. Created attachment 277215 [details]
Patch
Comment on attachment 277215 [details]
Patch
Looks like a good change.
I have these concerns:
1) A code change to shared code like this should not use the [WinCairo] prefix on the bug title.
2) The explanation covers why JSGlobalObjectRareData needs to be changed, but does not explain the Lock change at all.
3) Nothing has been done to prevent this kind of problem from happening in the future in the WinCairo port; other ports don’t depend so sensitively on the the use of WTF_MAKE_FAST_ALLOCATED. Someone needs to come up with a technique to prevent this kind of error from being reintroduced in the future.
Otherwise the patch looks OK to me.
Thank you for reviewing my patch. Created attachment 277226 [details]
Patch
Comment on attachment 277226 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277226&action=review Please fix the typos. > Source/JavaScriptCore/ChangeLog:8 > + WebKit uses CRT static libarary on Windows. Each copy of the CRT typo: /libarary/library/. > Source/JavaScriptCore/ChangeLog:23 > + the inlined consturctor of JSGlobalObjectRareData. typo: /consturctor/constructor/. > Source/WTF/ChangeLog:8 > + WebKit uses CRT static libarary on Windows. Each copy of the CRT Ditto. Please fix typo. > Source/WTF/ChangeLog:23 > + the inlined consturctor of JSGlobalObjectRareData. Please fix typo. Created attachment 277296 [details]
Patch
Thank you for r+. I have no permission to commit, created a new patch by fixing the typos.
Comment on attachment 277296 [details]
Patch
r=me
👍
Comment on attachment 277296 [details] Patch Clearing flags on attachment: 277296 Committed r200068: <http://trac.webkit.org/changeset/200068> All reviewed patches have been landed. Closing bug. |