358192 - Unbalanced js_Enter/LeaveLocalRootScope in js_ErrorToException

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

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]

Attachment: 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]]

Comment on attachment 243641 [details] [diff] [review]
Fix v1

Argh.

/be

Comment 3

[Igor Bukanov]

Attachment: 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]]

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]

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]

Attachment: Fix v3

Patch to commit with comments fixed.

Comment 7

[Igor Bukanov]

(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]]

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]

(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]

Attachment: Fix v2 for 1.8.0 branch

Non-trivial porting requires an extra review.

Comment 11

[Igor Bukanov]

Attachment: diff between Fix v2 and Fix v2 for 1.8.0

Comment 12

[Brendan Eich [:brendan]]

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]]

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]

(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]]

(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]]

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]

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]

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] (inactive)]

verified fixed 1.8.0.9, 1.8.1.1, 1.9 by code inspection.

Comment 20

[Alexander Sack]

... not applicable to 1.7 branch !?

Comment 21

[Igor Bukanov]

(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.