Bug 208978 – [JSC] Use CacheableIdentifier in ByValInfo

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

ByValInfo is also destroyed in CodeBlock::finalizeUnconditionally. And this function does not have AtomStringTable.
We must not destroy String here.

[Yusuke Suzuki]

<rdar://problem/60289819>

[Yusuke Suzuki]

Created attachment 393346 [details]
Patch

[Yusuke Suzuki]

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?

[Yusuke Suzuki]

Created attachment 393349 [details]
Patch

[Yusuke Suzuki]

Created attachment 393350 [details]
Patch

[Yusuke Suzuki]

jsc-i386 seems requiring clean build

[Yusuke Suzuki]

Created attachment 393357 [details]
Patch

[Saam Barati]

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?

[Keith Miller]

I was under the impression that finalizeUnconditionally happened on the mutator thread? Why is the atomic string table not initialized there?

[Keith Miller]

(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...

[Saam Barati]

(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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 393393 [details]
Patch

[Saam Barati]

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)

[Saam Barati]

Comment on attachment 393393 [details]
Patch

r=me

[Yusuke Suzuki]

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`.

[Yusuke Suzuki]

Committed r258344: <https://trac.webkit.org/changeset/258344>