Bug 206632 - InternalField and CheckNeutered DFG nodes are not always safe to execute
Summary: InternalField and CheckNeutered DFG nodes are not always safe to execute
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-01-22 17:20 PST by Keith Miller
Modified: 2020-01-23 11:01 PST (History)
7 users (show)

See Also:


Attachments
Patch (9.97 KB, patch)
2020-01-22 17:32 PST, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (8.13 KB, patch)
2020-01-22 17:48 PST, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (8.02 KB, patch)
2020-01-22 17:54 PST, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (8.08 KB, patch)
2020-01-22 18:26 PST, Keith Miller
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2020-01-22 17:20:38 PST
InternalField and CheckNeutered DFG nodes are not always safe to execute
Comment 1 Keith Miller 2020-01-22 17:32:14 PST
Created attachment 388493 [details]
Patch
Comment 2 Keith Miller 2020-01-22 17:38:23 PST
Comment on attachment 388493 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGNode.h:1401
> +        switch (op()) {
> +        case IsCellWithType:
> +        case GetInternalField:
> +        case PutInternalField:
> +            return true;
> +        default:
> +            return false;
> +        }
>      }
>  
>      JSType queriedType()
>      {
>          static_assert(std::is_same<uint8_t, std::underlying_type<JSType>::type>::value, "Ensure that uint8_t is the underlying type for JSType.");
> -        return static_cast<JSType>(m_opInfo.as<uint32_t>());
> +        if (op() == IsCellWithType)
> +            return static_cast<JSType>(m_opInfo.as<uint32_t>());
> +        return static_cast<JSType>(m_opInfo2.as<uint32_t>());

Didn't mean to upload this. Will delete.
Comment 3 Saam Barati 2020-01-22 17:43:17 PST
Comment on attachment 388493 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +        GetInternalField, etc. rely on a a proof that the cell passed to it is a subclass of InteralFieldObject

not necessarily a subclass of InternalFieltObject, but a subclass of a specific kind of InternalFieldObject, e.g, ArrayIterator

> Source/JavaScriptCore/ChangeLog:12
> +        Also, remove a bogus assertion that we will have proven the value passed to CheckNeutered is a TypedArray.

nit: maybe say why it's bogus.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1797
> +    MacroAssembler::JumpList speculationFailures;
> +    if (!speculationChecked(m_state.forNode(node->child1()).m_type, SpecTypedArrayView)) {
> +        GPRTemporary tmp(this);
> +        GPRReg tmpReg = tmp.gpr();
> +
> +        m_jit.load8(MacroAssembler::Address(baseReg, JSCell::typeInfoTypeOffset()), tmpReg);
> +        m_jit.sub32(TrustedImm32(FirstTypedArrayType), tmpReg);
> +        speculationFailures.append(m_jit.branch32(JITCompiler::AboveOrEqual, tmpReg, TrustedImm32(NumberOfTypedArrayTypesExcludingDataView)));
> +    }

we don't need this given your code inside SafeToExecute and how we emit this node only after CheckArray
Comment 4 Mark Lam 2020-01-22 17:48:30 PST
Comment on attachment 388493 [details]
Patch

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

> JSTests/stress/for-of-bad-internal-field-hoist.js:1
> +//requireOptions("--maximumFunctionForCallInlineCandidateBytecodeCost=500")

//@ requireOptions(...
Comment 5 Keith Miller 2020-01-22 17:48:52 PST
Created attachment 388499 [details]
Patch
Comment 6 Keith Miller 2020-01-22 17:54:05 PST
Created attachment 388500 [details]
Patch
Comment 7 Keith Miller 2020-01-22 18:17:32 PST
(In reply to Saam Barati from comment #3)
> Comment on attachment 388493 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=388493&action=review
> 
> > Source/JavaScriptCore/ChangeLog:9
> > +        GetInternalField, etc. rely on a a proof that the cell passed to it is a subclass of InteralFieldObject
> 
> not necessarily a subclass of InternalFieltObject, but a subclass of a
> specific kind of InternalFieldObject, e.g, ArrayIterator

Uhh, I guess I meant as a C++ subclass not a JS subclass. The Get/SetInternalField nodes don't know which one they are loading from at either the bytecode or DFG level.
Comment 8 Saam Barati 2020-01-22 18:18:18 PST
Comment on attachment 388500 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:10
> +        GetInternalField, etc. rely on a a proof that the cell passed to it is a subclass of InteralFieldObject
> +        but we may hoist it past the check guarding it.

nit: sentence is a bit of a run on and can be made clearer

> Source/JavaScriptCore/ChangeLog:13
> +        It's not valid to require that AI will preserve any invariant since phases can make changes that AI doesn't

"preserve" isn't the right word here. AI won't break the program. But it might not be able to precisely model it.

> Source/JavaScriptCore/dfg/DFGSafeToExecute.h:38
> +// not prove it is valid. Thus, it is always 

you forgot to finish your comment here
Comment 9 Keith Miller 2020-01-22 18:26:10 PST
Created attachment 388503 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2020-01-22 18:58:57 PST
Comment on attachment 388503 [details]
Patch for landing

Clearing flags on attachment: 388503

Committed r254959: <https://trac.webkit.org/changeset/254959>
Comment 11 WebKit Commit Bot 2020-01-22 18:58:59 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2020-01-22 18:59:13 PST
<rdar://problem/58820894>
Comment 13 Mark Lam 2020-01-23 11:01:30 PST
<rdar://problem/58450906>