Last Comment Bug 382253 - interpreter uses goto out where it should jump to bad_inline_call
: interpreter uses goto out where it should jump to bad_inline_call
Status: RESOLVED FIXED
[sg:moderate?]
: fixed1.8.1.5
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Igor Bukanov
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-05-28 12:19 PDT by Igor Bukanov
Modified: 2007-08-23 14:25 PDT (History)
4 users (show)
dveditz: blocking1.8.1.5+
dveditz: wanted1.8.1.x+
dveditz: wanted1.8.0.x-
bob: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix v1 (123 bytes, patch)
2007-05-28 12:19 PDT, Igor Bukanov
no flags Details | Diff | Splinter Review
fix v1 for real (2.93 KB, patch)
2007-05-28 12:21 PDT, Igor Bukanov
no flags Details | Diff | Splinter Review
fix v2 (2.92 KB, patch)
2007-06-01 00:34 PDT, Igor Bukanov
brendan: review+
Details | Diff | Splinter Review
fix v2 for 1.8.1 (2.88 KB, patch)
2007-07-10 05:16 PDT, Igor Bukanov
dveditz: review+
dveditz: approval1.8.1.5+
Details | Diff | Splinter Review
diff between trunk and 1.8.1 versions (757 bytes, text/plain)
2007-07-10 05:18 PDT, Igor Bukanov
no flags Details

Description Igor Bukanov 2007-05-28 12:19:23 PDT
Created attachment 266411 [details] [diff] [review]
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 2007-05-28 12:21:03 PDT
Created attachment 266412 [details] [diff] [review]
fix v1 for real
Comment 2 Brendan Eich [:brendan] 2007-05-29 23:51:04 PDT
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] 2007-05-30 14:10:24 PDT
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 2007-05-31 03:53:05 PDT
(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 2007-06-01 00:34:14 PDT
Created attachment 266880 [details] [diff] [review]
fix v2

The new version unconditionally calls js_FreeRawStack.
Comment 6 Igor Bukanov 2007-06-01 20:47:38 PDT
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] 2007-07-09 15:53:28 PDT
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 2007-07-10 05:16:40 PDT
Created attachment 271655 [details] [diff] [review]
fix v2 for 1.8.1

This is a straightforward hand-merge of fix v2 with 1.8.1 branch.
Comment 9 Igor Bukanov 2007-07-10 05:18:07 PDT
Created attachment 271656 [details]
diff between trunk and 1.8.1 versions
Comment 10 Igor Bukanov 2007-07-10 05:26:00 PDT
The bug does not exist on 1.8.0 branch as those problematic goto out were added later.
Comment 11 Daniel Veditz [:dveditz] 2007-07-10 10:47:38 PDT
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 2007-07-10 11:15:29 PDT
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

Note You need to log in before you can comment on or make changes to this bug.