382253 - interpreter uses goto out where it should jump to bad_inline_call

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

Attachment: fix v1

The code in the interpreter implementing the inline call logic in couple of places uses "goto out" where it should use "goto bad_inline_call".

This is exploitable if a script can trigger object allocation failure during inline call. In this case the interpreter will goto to the exception handling code where it would use pc from the parent frame while script and other registers will be set to the new frame.

But it is rather unlikely that such precise control of memory allocation is feasible.

Comment 1

[Igor Bukanov]

Attachment: fix v1 for real

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 266412 [details] [diff] [review]
fix v1 for real

>               bad_inline_call:
>+                RESTORE_SP(fp);
>+                JS_ASSERT(fp->pc == pc);
>                 script = fp->script;
>                 depth = (jsint) script->depth;
>                 atoms = script->atomMap.vector;
>+                if (newsp)

This should test newmark, not newsp (which is non-null here, always), right?

>+                    js_FreeRawStack(cx, newmark);
>                 ok = JS_FALSE;
>                 goto out;
>             }
> 
>             ok = js_Invoke(cx, argc, 0);
>             RESTORE_SP(fp);
>             LOAD_INTERRUPT_HANDLER(rt);

Comment 3

[Brendan Eich [:brendan]]

Hrm, newmark is also guaranteed non-null. Why test at all? Does any goto come from a site where the stack marked by newmark has not been allocated, and needs to be freed?

/be

Comment 4

[Igor Bukanov]

(In reply to comment #3)
> Hrm, newmark is also guaranteed non-null. Why test at all? 

You are right, the test should not be done and just resetting the arena back to the  value stored in newmark will do the error recovery with or without JS_ARENA_ALLOCATE_CAST(newsp, ...) failed at http://lxr.mozilla.org/seamonkey/source/js/src/jsinterp.c#3838 .

I will update the patch later today.

Comment 5

[Igor Bukanov]

Attachment: fix v2

The new version unconditionally calls js_FreeRawStack.

Comment 6

[Igor Bukanov]

I committed the patch from comment 4 to the trunk:

Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.350; previous revision: 3.349
done

Comment 7

[Daniel Veditz [:dveditz]]

Comment on attachment 266880 [details] [diff] [review]
fix v2

Will we need another patch for the 1.8 branch or does this fix the problem? Please request approval or attach a new patch (code freeze for 1.8.1.5 is July 13).  Thanks!

Comment 8

[Igor Bukanov]

Attachment: fix v2 for 1.8.1

This is a straightforward hand-merge of fix v2 with 1.8.1 branch.

Comment 9

[Igor Bukanov]

Attachment: diff between trunk and 1.8.1 versions

Comment 10

[Igor Bukanov]

The bug does not exist on 1.8.0 branch as those problematic goto out were added later.

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 271655 [details] [diff] [review]
fix v2 for 1.8.1

r=dveditz (rubber-stamp for effectively identical patches)

approved for 1.8.1.5, a=dveditz for release-drivers

Comment 12

[Igor Bukanov]

I committed the patch from comment 8 to MOZILLA_1_8_BRANCH:

Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.181.2.91; previous revision: 3.181.2.90
done