Bug 182006 - Use precise index masking for FTL GetByArgumentByVal
Summary: Use precise index masking for FTL GetByArgumentByVal
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords: InRadar
Depends on:
Blocks: 182419
  Show dependency treegraph
 
Reported: 2018-01-23 12:55 PST by Filip Pizlo
Modified: 2018-02-01 20:56 PST (History)
6 users (show)

See Also:


Attachments
the patch (7.48 KB, patch)
2018-01-23 14:21 PST, Filip Pizlo
keith_miller: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2018-01-23 12:55:40 PST
rdar://problem/36286370
Comment 1 Filip Pizlo 2018-01-23 14:21:51 PST
Created attachment 332074 [details]
the patch
Comment 2 Radar WebKit Bug Importer 2018-01-23 14:22:13 PST
<rdar://problem/36793736>
Comment 3 Mark Lam 2018-01-23 14:33:30 PST
<rdar://problem/36286370>
Comment 4 Keith Miller 2018-01-23 15:28:10 PST
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.
Comment 5 Filip Pizlo 2018-01-23 15:29:38 PST
(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.
Comment 6 Filip Pizlo 2018-01-23 16:40:44 PST
Landed in https://trac.webkit.org/changeset/227462/webkit
Comment 7 Saam Barati 2018-02-01 14:39:50 PST
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
Comment 8 Saam Barati 2018-02-01 14:54:25 PST
(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.
Comment 9 Mark Lam 2018-02-01 20:55:45 PST
(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.