Closed Bug 1054047 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: nbp, Assigned: nbp)

References

Details

(Keywords: assertion, testcase)

Attachments

(1 file)

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 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.
Attachment #8473263 - Flags: review?(sunfish)
(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.
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);
Keywords: assertion, testcase
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.
Attachment #8473263 - Flags: review?(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.
Attachment #8473263 - Flags: review?(sunfish) → review+
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.
Blocks: 950764
Group: javascript-core-security
Keywords: sec-other
https://hg.mozilla.org/mozilla-central/rev/dfddabf968de
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.