ByValInfo is also destroyed in CodeBlock::finalizeUnconditionally. And this function does not have AtomStringTable. We must not destroy String here.
<rdar://problem/60289819>
Created attachment 393346 [details] Patch
Comment on attachment 393346 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393346&action=review > Source/JavaScriptCore/heap/Heap.cpp:1508 > + auto* previous = Thread::current().setCurrentAtomStringTable(nullptr); > + auto scopeExit = makeScopeExit([&] { > + Thread::current().setCurrentAtomStringTable(previous); > + }); Should we do that only for debug builds?
Created attachment 393349 [details] Patch
Created attachment 393350 [details] Patch
jsc-i386 seems requiring clean build
Created attachment 393357 [details] Patch
Comment on attachment 393357 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393357&action=review > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:7580 > + identifierNumber = m_graph.identifiers().ensure(byValInfo->cachedId.uid()); you need to freeze this cell regardless, no? Otherwise, potential UAF if the byValInfo gets cleared > Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1295 > + slowCases.append(branchPtr(NotEqual, scratch, TrustedImmPtr(byValInfo->cachedId.uid()))); why is this not a possible UAF? Is it because we only ever set cacheable identifier once? > Source/JavaScriptCore/runtime/CacheableIdentifier.h:99 > + uintptr_t m_bits { 0 }; why?
I was under the impression that finalizeUnconditionally happened on the mutator thread? Why is the atomic string table not initialized there?
(In reply to Keith Miller from comment #9) > I was under the impression that finalizeUnconditionally happened on the > mutator thread? Why is the atomic string table not initialized there? Oh nvm, should have read the updated ChangeLog...
(In reply to Keith Miller from comment #9) > I was under the impression that finalizeUnconditionally happened on the > mutator thread? Why is the atomic string table not initialized there? finalizeUnconditionally happens with the mutator stopped. Not necessarily on the mutator. I suppose a different implementation could be to swap in the mutator's atomic string table? But I kinda like cacheable identifier more.
Comment on attachment 393357 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393357&action=review >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:7580 >> + identifierNumber = m_graph.identifiers().ensure(byValInfo->cachedId.uid()); > > you need to freeze this cell regardless, no? Otherwise, potential UAF if the byValInfo gets cleared Oops, right. >> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1295 >> + slowCases.append(branchPtr(NotEqual, scratch, TrustedImmPtr(byValInfo->cachedId.uid()))); > > why is this not a possible UAF? Is it because we only ever set cacheable identifier once? Yes. >> Source/JavaScriptCore/runtime/CacheableIdentifier.h:99 >> + uintptr_t m_bits { 0 }; > > why? Otherwise, `CacheableIdentifier value;`'s m_bits is undefined.
Created attachment 393393 [details] Patch
Comment on attachment 393357 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393357&action=review >>> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1295 >>> + slowCases.append(branchPtr(NotEqual, scratch, TrustedImmPtr(byValInfo->cachedId.uid()))); >> >> why is this not a possible UAF? Is it because we only ever set cacheable identifier once? > > Yes. Can we document this inside ByValInfo's header as a requirement? >>> Source/JavaScriptCore/runtime/CacheableIdentifier.h:99 >>> + uintptr_t m_bits { 0 }; >> >> why? > > Otherwise, `CacheableIdentifier value;`'s m_bits is undefined. why not delete the default constructor? Do we use it? (Sorry, I wrote this assuming it was deleted, but maybe it isn't)
Comment on attachment 393393 [details] Patch r=me
Comment on attachment 393357 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393357&action=review Thanks! >>>> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1295 >>>> + slowCases.append(branchPtr(NotEqual, scratch, TrustedImmPtr(byValInfo->cachedId.uid()))); >>> >>> why is this not a possible UAF? Is it because we only ever set cacheable identifier once? >> >> Yes. > > Can we document this inside ByValInfo's header as a requirement? Yeah, I've just documented. >>>> Source/JavaScriptCore/runtime/CacheableIdentifier.h:99 >>>> + uintptr_t m_bits { 0 }; >>> >>> why? >> >> Otherwise, `CacheableIdentifier value;`'s m_bits is undefined. > > why not delete the default constructor? Do we use it? (Sorry, I wrote this assuming it was deleted, but maybe it isn't) ByValInfo is using this right now to make the initial one `nullptr`.
Committed r258344: <https://trac.webkit.org/changeset/258344>