The default bug view has changed. See this FAQ.

interpreter uses goto out where it should jump to bad_inline_call

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

({fixed1.8.1.5})

unspecified
fixed1.8.1.5
Points:
---
Bug Flags:
blocking1.8.1.5 +
wanted1.8.1.x +
wanted1.8.0.x -
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:moderate?])

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

10 years ago
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.
Flags: blocking1.8.1.5?
Flags: blocking1.8.0.13?
(Assignee)

Updated

10 years ago
Attachment #266411 - Flags: review?(brendan)
(Assignee)

Comment 1

10 years ago
Created attachment 266412 [details] [diff] [review]
fix v1 for real
Attachment #266411 - Attachment is obsolete: true
Attachment #266412 - Flags: review?(brendan)
Attachment #266411 - Flags: review?(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);
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
(Assignee)

Comment 4

10 years ago
(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.
(Assignee)

Comment 5

10 years ago
Created attachment 266880 [details] [diff] [review]
fix v2

The new version unconditionally calls js_FreeRawStack.
Attachment #266412 - Attachment is obsolete: true
Attachment #266880 - Flags: review?(brendan)
Attachment #266412 - Flags: review?(brendan)

Updated

10 years ago
Attachment #266880 - Flags: review?(brendan) → review+
(Assignee)

Comment 6

10 years ago
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
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Flags: blocking1.8.1.5?
Flags: blocking1.8.1.5+
Flags: blocking1.8.0.13?
Flags: blocking1.8.0.13+
Whiteboard: [sg:moderate?]

Updated

10 years ago
Flags: in-testsuite-
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
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!
(Assignee)

Comment 8

10 years ago
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.
Attachment #266880 - Attachment is obsolete: true
Attachment #271655 - Flags: review?(brendan)
Attachment #271655 - Flags: approval1.8.1.5?
(Assignee)

Comment 9

10 years ago
Created attachment 271656 [details]
diff between trunk and 1.8.1 versions
(Assignee)

Updated

10 years ago
Attachment #271656 - Attachment mime type: text/x-patch → text/plain
(Assignee)

Comment 10

10 years ago
The bug does not exist on 1.8.0 branch as those problematic goto out were added later.
Flags: blocking1.8.0.13+
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
Attachment #271655 - Flags: review?(brendan)
Attachment #271655 - Flags: review+
Attachment #271655 - Flags: approval1.8.1.5?
Attachment #271655 - Flags: approval1.8.1.5+
(Assignee)

Comment 12

10 years ago
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
Keywords: fixed1.8.1.5
Group: security
Flags: wanted1.8.0.x+ → wanted1.8.0.x-
You need to log in before you can comment on or make changes to this bug.