Bug 182006 – Use precise index masking for FTL GetByArgumentByVal

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

[Filip Pizlo]

rdar://problem/36286370

[Filip Pizlo]

Created attachment 332074 [details]
the patch

[Radar WebKit Bug Importer]

<rdar://problem/36793736>

[Mark Lam]

<rdar://problem/36286370>

[Keith Miller]

Comment on attachment 332074 [details]
the patch

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

r=me with comments.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3987
> +        LValue limit = m_out.sub(originalLimit, m_out.int32One);

Nit: I would make this 

LValue thisSize = m_out.int32One;
LValue limit = m_out.sub(originalLimit, thisSize);

I feel like I'll forget that the -1 is for this.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4009
> +        index = m_out.add(index, m_out.constInt32(1));

Nit: m_out.constInt32(1) => m_out.int32One.

Also, ditto on the thisSize.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4017
> +        index = m_out.bitAnd(
> +            index,
> +            m_out.aShr(
> +                m_out.sub(
> +                    index,
> +                    m_out.opaque(originalLimit)),
> +                m_out.constInt32(31)));

Nit: I would do this math on 64-bit values. That avoids people passing very large 32-bit indices and causes issues here.

E.g. arguments[UINT_MAX - epsilon] would produce a non-zero mask.

[Filip Pizlo]

(In reply to Keith Miller from comment #4)
> Comment on attachment 332074 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=332074&action=review
> 
> r=me with comments.
> 
> > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3987
> > +        LValue limit = m_out.sub(originalLimit, m_out.int32One);
> 
> Nit: I would make this 
> 
> LValue thisSize = m_out.int32One;
> LValue limit = m_out.sub(originalLimit, thisSize);
> 
> I feel like I'll forget that the -1 is for this.
> 
> > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4009
> > +        index = m_out.add(index, m_out.constInt32(1));
> 
> Nit: m_out.constInt32(1) => m_out.int32One.

Oops.  Fixed.

> 
> Also, ditto on the thisSize.
> 
> > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4017
> > +        index = m_out.bitAnd(
> > +            index,
> > +            m_out.aShr(
> > +                m_out.sub(
> > +                    index,
> > +                    m_out.opaque(originalLimit)),
> > +                m_out.constInt32(31)));
> 
> Nit: I would do this math on 64-bit values. That avoids people passing very
> large 32-bit indices and causes issues here.
> 
> E.g. arguments[UINT_MAX - epsilon] would produce a non-zero mask.

Good call.  I'll try that.

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/227462/webkit

[Saam Barati]

Comment on attachment 332074 [details]
the patch

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

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3990
> +        if (m_node->numberOfArgumentsToSkip())
> +            limit = m_out.sub(limit, m_out.constInt32(m_node->numberOfArgumentsToSkip()));

How did this fix an overflow situation? It introduced an overflow situation.
Both limit and numberOfArugmnetsToSkip are user controlled. For example:

```
function foo(a, b, ...args) {
    return args[0];
}
foo(10);
```

here,
limit = 0
argsToSkip = 2
limit - argsToSkip = bad time

[Saam Barati]

(In reply to Saam Barati from comment #7)
> Comment on attachment 332074 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=332074&action=review
> 
> > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3990
> > +        if (m_node->numberOfArgumentsToSkip())
> > +            limit = m_out.sub(limit, m_out.constInt32(m_node->numberOfArgumentsToSkip()));
> 
> How did this fix an overflow situation? It introduced an overflow situation.
> Both limit and numberOfArugmnetsToSkip are user controlled. For example:
> 
> ```
> function foo(a, b, ...args) {
>     return args[0];
> }
> foo(10);
> ```
> 
> here,
> limit = 0
> argsToSkip = 2
> limit - argsToSkip = bad time

What we need here is to go back to the old code, but to perform an overflow check on the original add. Or we need to do some branch before this sub. Either would work.

[Mark Lam]

(In reply to Saam Barati from comment #8)
> > here,
> > limit = 0
> > argsToSkip = 2
> > limit - argsToSkip = bad time
> 
> What we need here is to go back to the old code, but to perform an overflow
> check on the original add. Or we need to do some branch before this sub.
> Either would work.

We're fixing this in https://bugs.webkit.org/show_bug.cgi?id=182419.