1054047 - MArgumentsLength::computeRange should return the ActualArgs limit instead of the Formal args limit.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

So far I did not managed to do any out-of-bound access because of the lack of use of our range analysis information for folding bounds check, otherwise some code such as:


var test = [];
function f() {
    var x = 128 - arguments.length; // inferred range [1, 128]
    for (var j = 0; j < 130; j++) {
        test[j] = 0;       // bound check [0, 130]
        test[x] = 123456;  // bound check [1, 128]
    }
}

function g() {
    f.apply(this, arguments);
}

for (var j = 0; j < 130; j++)
    test.push(0);

var arr = [];
for (var j = 0; j < 128; j++)
    arr.push(0);

for (var j = 0; j < 10000; j++) {
    g.apply(null, arr);
}

var off = -2;
while (128 - arr.length != off)
    arr.push(0);
var x = arr.length;
g.apply(null, arr);
assertEq(arr.length, x);


would have the potential for building an exploit by mapping all the memory into one Array.

In the mean time, I still prefer to open this bug as a secure bug as I don't know if an exploit could be made out of it, even if I failed at doing so.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: Determine the correct range from MArgumentLength.

Comment 2

[Dan Gohman [:sunfish]]

Comment on attachment 8473263 [details] [diff] [review]
Determine the correct range from MArgumentLength.

Review of attachment 8473263 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/RangeAnalysis.cpp
@@ +1523,5 @@
> +    // This is is a conservative upper bound on what |TooManyActualArguments|
> +    // checks.  If exceeded, Ion will not be entered in the first place.
> +    MOZ_ASSERT(js_JitOptions.maxStackArgs <= UINT32_MAX,
> +               "NewUInt32Range requires a uint32 value");
> +    setRange(Range::NewUInt32Range(alloc, 0, js_JitOptions.maxStackArgs));

js_JitOptions.maxStackArgs is enforced like this:

inline bool
TooManyArguments(unsigned nargs)
{
    return nargs >= SNAPSHOT_MAX_NARGS || nargs > js_JitOptions.maxStackArgs;
}

From this, it looks like there isn't actually a bug here. SNAPSHOT_MAX_NARGS should indeed be conservatively correct.

It does look like you could make it more aggressive though. It looks like a tighter upper bound is Min(SNAPSHOT_MAX_NARGS - 1, js_JitOptions.maxStackArgs), assuming I haven't misread anything here.

Comment 3

[Nicolas B. Pierron [:nbp]]

(In reply to Dan Gohman [:sunfish] from comment #2)
> Comment on attachment 8473263 [details] [diff] [review]
> Determine the correct range from MArgumentLength.
> 
> Review of attachment 8473263 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/RangeAnalysis.cpp
> @@ +1523,5 @@
> > +    // This is is a conservative upper bound on what |TooManyActualArguments|
> > +    // checks.  If exceeded, Ion will not be entered in the first place.
> > +    MOZ_ASSERT(js_JitOptions.maxStackArgs <= UINT32_MAX,
> > +               "NewUInt32Range requires a uint32 value");
> > +    setRange(Range::NewUInt32Range(alloc, 0, js_JitOptions.maxStackArgs));
> 
> js_JitOptions.maxStackArgs is enforced like this:
> 
> inline bool
> TooManyArguments(unsigned nargs)
> {
>     return nargs >= SNAPSHOT_MAX_NARGS || nargs > js_JitOptions.maxStackArgs;
> }
> 
> From this, it looks like there isn't actually a bug here. SNAPSHOT_MAX_NARGS
> should indeed be conservatively correct.
> 
> It does look like you could make it more aggressive though. It looks like a
> tighter upper bound is Min(SNAPSHOT_MAX_NARGS - 1,
> js_JitOptions.maxStackArgs), assuming I haven't misread anything here.

Have a look at Bug 950764.

TooManyArguments is supposed to help us guard that we do not have too many arguments, for 2 reasons, bloating up the stack space (TooManyActualArgs) or the compilation (TooManyFormalArgs).

The range is not the Min, but the Max of both, which means that we should probably assert that SNAPSHOT_MAX_ARGS <= js_JitOptions.maxStackArgs.

Comment 4

[Nicolas B. Pierron [:nbp]]

The following test case assert with --ion-offthread-compile=off --ion-check-range-analysis:


function f() {}
function g() {
    f.apply(this, arguments);
}

var arr = [];
for (var j = 0; j < 128 /* 127 */; j++)
    arr.push(0);

for (var j = 0; j < 10000; j++)
    g.apply(null, arr);

Comment 5

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8473263 [details] [diff] [review]
Determine the correct range from MArgumentLength.

Review of attachment 8473263 [details] [diff] [review]:
-----------------------------------------------------------------

re-ask for review based on comment 3 & 4.
This patch get rid of the assertion message.

Comment 6

[Dan Gohman [:sunfish]]

Comment on attachment 8473263 [details] [diff] [review]
Determine the correct range from MArgumentLength.

Review of attachment 8473263 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/RangeAnalysis.cpp
@@ +1523,5 @@
> +    // This is is a conservative upper bound on what |TooManyActualArguments|
> +    // checks.  If exceeded, Ion will not be entered in the first place.
> +    MOZ_ASSERT(js_JitOptions.maxStackArgs <= UINT32_MAX,
> +               "NewUInt32Range requires a uint32 value");
> +    setRange(Range::NewUInt32Range(alloc, 0, js_JitOptions.maxStackArgs));

Ok, with the patch in bug 950764, this makes sense.

Comment 7

[Nicolas B. Pierron [:nbp]]

Bug 801921 introduced the current range which inferred from the maximal length of the argument vector visible in Ion, which is coming from the stack limitation of Ion.

This number got restricted by js_JitOptions.maxStackArgs (add as part of Bug 759442), and got many time miss interpreted as being SNAPSHOT_MAX_ARGS, which is the limit made up for the formal arguments, which does not have to correspond to the limit of the actual arguments.

But apparently, this was not possible without patches from Bug 950764.  So the range analysis was correct by mistake. :D

Test case from comment 4 and comment 0 do not assert.

=> Opening this bug and marking it as a blocker of Bug 950764.

Comment 8

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dfddabf968de

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/dfddabf968de