Last Comment Bug 358192 - Unbalanced js_Enter/LeaveLocalRootScope in js_ErrorToException
: Unbalanced js_Enter/LeaveLocalRootScope in js_ErrorToException
Status: VERIFIED FIXED
[sg:moderate]
: verified1.8.0.9, verified1.8.1.1
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Igor Bukanov
:
Mentors:
Depends on: 347248
Blocks: js1.7src
  Show dependency treegraph
 
Reported: 2006-10-26 07:58 PDT by Igor Bukanov
Modified: 2006-12-27 12:27 PST (History)
4 users (show)
dveditz: blocking1.8.1.1+
dveditz: blocking1.8.0.9+
bob: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix v1 (3.31 KB, patch)
2006-10-26 08:21 PDT, Igor Bukanov
brendan: review+
Details | Diff | Review
Fix v2 (3.68 KB, patch)
2006-10-26 15:06 PDT, Igor Bukanov
brendan: review+
dveditz: approval1.8.1.1+
Details | Diff | Review
Fix v3 (31.69 KB, patch)
2006-10-26 16:19 PDT, Igor Bukanov
igor: review+
Details | Diff | Review
Fix v2 for 1.8.0 branch (3.63 KB, patch)
2006-11-06 16:27 PST, Igor Bukanov
brendan: review+
dveditz: approval1.8.0.9+
Details | Diff | Review
diff between Fix v2 and Fix v2 for 1.8.0 (2.67 KB, patch)
2006-11-06 16:29 PST, Igor Bukanov
no flags Details | Diff | Review

Description Igor Bukanov 2006-10-26 07:58:57 PDT
The code in js_ErrorToException from jsexn.c contains:

    /* Protect the newly-created strings below from nesting GCs. */
    ok = js_EnterLocalRootScope(cx);
    if (!ok)
        goto out;
...
  out:
    js_LeaveLocalRootScope(cx);

That is, it calls js_LeaveLocalRootScope even when the corresponding js_EnterLocalRootScope fails.

Since on tight memory it can be used to remove local root protection, I mark the bug as security-related.
Comment 1 Igor Bukanov 2006-10-26 08:21:37 PDT
Created attachment 243641 [details] [diff] [review]
Fix v1

The patch changes js_ErrorToException to use temp roots instead of EnterLocalRoot which works since JS_PUSH_TEMP_ROOT cannot fail.

Effectively the patch changes js_ErrorToException to the AVIARY_1_0_1_01252004_BRANCH version since there EnterLocalRoot is not available so the fix for bug 312278 used JS_PUSH_TEMP_ROOT. The only difference compared with AVIARY_1_0_1_01252004_BRANCH is that the patch is slightly paranoid and uses 4 temp roots instead of 2 to ensure the same rooting properties as EnterLocalRoot provides.
Comment 2 Brendan Eich [:brendan] 2006-10-26 13:27:58 PDT
Comment on attachment 243641 [details] [diff] [review]
Fix v1

Argh.

/be
Comment 3 Igor Bukanov 2006-10-26 15:06:16 PDT
Created attachment 243713 [details] [diff] [review]
Fix v2

Compared with v1, this patch moves JS_SetPendingException after the exception object initialization code. In this way an Out-of-Memory error during the initialization would not be catchable by scripts.

Previously the early call to JS_SetPendingException was done to root the exception. But this is not necessary since the object is rooted in any case.
Comment 4 Brendan Eich [:brendan] 2006-10-26 15:13:02 PDT
Comment on attachment 243713 [details] [diff] [review]
Fix v2

I should have seen that.  Nice reordering of errorString's decl too.

/be
Comment 5 Igor Bukanov 2006-10-26 15:26:49 PDT
I committed the patch from comment 3 to the trunk:

Checking in jsexn.c;
/cvsroot/mozilla/js/src/jsexn.c,v  <--  jsexn.c
new revision: 3.79; previous revision: 3.78
done
Comment 6 Igor Bukanov 2006-10-26 16:19:39 PDT
Created attachment 243728 [details] [diff] [review]
Fix v3

Patch to commit with comments fixed.
Comment 7 Igor Bukanov 2006-10-26 16:21:15 PDT
(In reply to comment #6)
> Created an attachment (id=243728) [edit]
> Fix v3
> 
> Patch to commit with comments fixed.
> 

Please ignore this: wrong bug!
Comment 8 Daniel Veditz [:dveditz] 2006-11-06 10:32:18 PST
Is "fix v2" ready for the braches, or will we need a merged/ported version? Please ask for "approval?" on the appropriate patch.
Comment 9 Igor Bukanov 2006-11-06 16:10:04 PST
(In reply to comment #8)
> Is "fix v2" ready for the braches, or will we need a merged/ported version?
> Please ask for "approval?" on the appropriate patch.

The patch would apply to 1.8.1 branch as is if one applies the patch from bug 347248 first. 1.8.0 on the other hand requires some porting.
Comment 10 Igor Bukanov 2006-11-06 16:27:15 PST
Created attachment 244842 [details] [diff] [review]
Fix v2 for 1.8.0 branch

Non-trivial porting requires an extra review.
Comment 11 Igor Bukanov 2006-11-06 16:29:47 PST
Created attachment 244843 [details] [diff] [review]
diff between Fix v2 and Fix v2 for 1.8.0
Comment 12 Brendan Eich [:brendan] 2006-11-06 19:58:55 PST
Comment on attachment 244842 [details] [diff] [review]
Fix v2 for 1.8.0 branch

Looks good -- too bad JS_ARRAY_LENGTH is not on the 1.8.0 branch.

/be
Comment 13 Daniel Veditz [:dveditz] 2006-11-07 14:43:10 PST
Calling this "sg:moderate" because the exploitable conditions seem hard to set up.

I'm happy taking these patches, but given the tight schedule for 2.0.0.1 (1.8.1.1) we're uncomfortable taking bug 347248 along with it. Can the two bugs be done separately, or are there extra reviews or testing that can reassure us about bug 347248?
Comment 14 Igor Bukanov 2006-11-10 05:16:33 PST
(In reply to comment #13)
> Calling this "sg:moderate" because the exploitable conditions seem hard to set
> up.
> 
> I'm happy taking these patches, but given the tight schedule for 2.0.0.1
> (1.8.1.1) we're uncomfortable taking bug 347248 along with it. Can the two bugs
> be done separately, 

Yes: in fact the patch 1.8.0 is almost the patch for 1.8.1 as well if 347248 would not be taken in.

> or are there extra reviews or testing that can reassure us
> about bug 347248?

Well, the optimization patch affects the frequently executed code path. 

Yes, in the initial form I introduced a regression there, but it was very quickly pointed-out due-to this frequently executed property. Moreover, the regression caused to consider the patch few more times by me and Brendan adding the confidence on top of the fact that the patch exists on the trunk with all regression fixed for more then 2 months.
Comment 15 Brendan Eich [:brendan] 2006-11-10 08:03:02 PST
(In reply to comment #13)
> Calling this "sg:moderate" because the exploitable conditions seem hard to set
> up.
> 
> I'm happy taking these patches, but given the tight schedule for 2.0.0.1
> (1.8.1.1) we're uncomfortable taking bug 347248 along with it. Can the two bugs
> be done separately, or are there extra reviews or testing that can reassure us
> about bug 347248?

Bug 347248 is well-baked.  Recommend you take it and not make yet another variation ("almost the same as 1.8.0" is not the same) to engineer and support.  Take the known quantity.

/be
Comment 16 Daniel Veditz [:dveditz] 2006-11-10 10:42:42 PST
Comment on attachment 243713 [details] [diff] [review]
Fix v2

approved for 1.8/1.8.0 branch, a=dveditz for drivers
Comment 17 Igor Bukanov 2006-11-22 00:20:28 PST
I committed the patch from comment 3 to MOZILLA_1_8_BRANCH:

Checking in jsexn.c;
/cvsroot/mozilla/js/src/jsexn.c,v  <--  jsexn.c
new revision: 3.50.4.11; previous revision: 3.50.4.10
done
Comment 18 Igor Bukanov 2006-11-22 01:11:15 PST
I committed the patch from comment 10 to MOZILLA_1_8_0_BRANCH:

Checking in jsexn.c;
/cvsroot/mozilla/js/src/jsexn.c,v  <--  jsexn.c
new revision: 3.50.4.4.2.4; previous revision: 3.50.4.4.2.3
done
Comment 19 Bob Clary [:bc:] 2006-11-27 23:18:05 PST
verified fixed 1.8.0.9, 1.8.1.1, 1.9 by code inspection.
Comment 20 Alexander Sack 2006-12-27 09:55:25 PST
... not applicable to 1.7 branch !?
Comment 21 Igor Bukanov 2006-12-27 12:27:01 PST
(In reply to comment #20)
> ... not applicable to 1.7 branch !?

No, js_EnterLocalRootScope API was introduced for 1.8.* and later. To root temporaries 1.7 branch uses JS_PUSH_TEMP_ROOT API and the corresponding line never fails.   

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