443871 - Protect against null objects in a title's scope/map

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

shaver, jorendorff, and brendan helped figure this out when i kept crashing here working on bug 437152. Apparently the assumption that a scope will always have an object is invalid. Patch attached.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 328296 [details] [diff] [review]
Patch

>+    title->ownercx = NULL;  /* NB: set last, after lock init */

That comment means what it says. This should not be moved up to common it in the early return if !obj and fall through cases. Instead, you want an if (obj) {...} around the whole js_MakeStringImmutable for loop.

/be

>+    JS_RUNTIME_METER(cx->runtime, sharedTitles);
>+
>     obj = scope->object;
>+    if (!obj)
>+        return;
>+
>     nslots = scope->map.freeslot;
>     for (i = 0; i != nslots; ++i) {
>         v = STOBJ_GET_SLOT(obj, i);
>         if (JSVAL_IS_STRING(v) &&
>             !js_MakeStringImmutable(cx, JSVAL_TO_STRING(v))) {
>             /*
>              * FIXME bug 363059: The following error recovery changes runtime
>              * execution semantics, arbitrarily and silently ignoring errors
>              * except out-of-memory, which should have been reported through
>              * JS_ReportOutOfMemory at this point.
>              */
>             STOBJ_SET_SLOT(obj, i, JSVAL_VOID);
>         }
>     }
>-
>-    title->ownercx = NULL;  /* NB: set last, after lock init */
>-    JS_RUNTIME_METER(cx->runtime, sharedTitles);
> }
> 
> /*
>  * Given a title with apparently non-null ownercx different from cx, try to
>  * set ownercx to cx, claiming exclusive (single-threaded) ownership of title.
>  * If we claim ownership, return true.  Otherwise, we wait for ownercx to be
>  * set to null (indicating that title is multi-threaded); or if waiting would
>  * deadlock, we set ownercx to null ourselves via ShareTitle.  In any case,

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v2

Updated to address brendan's comments.

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 330564 [details] [diff] [review]
Patch, v2

I mentally diff -w'ed and it looks good ;-).

/be

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Pushed to moz-central.

Comment 6

[Wladimir Palant]

For reference, the relevant revision is http://hg.mozilla.org/mozilla-central/rev/5f878cdd1d5e

Comment 7

[Igor Bukanov]

Most likely this bug is the reason for the top crash in bug 506823. Thus I suggest to backport it to 1.9.0. 

For the record, here is a scenario for this bug. scope->object becomes null when the object is GC-ed while the scope is shared among other reachable objects. This is possible, for example, for compiled functions. For them the code uses STOBJ_CLEAR_PARENT not to leak the compilation-time scope. But that still leaves Function.prototype scope shared. If that compile-time function is accessed from several threads, then we will get a bug like this since JS_OBJ_LOCK eventually calls js_FinishSharingTitle on such title and crash when the function tries to enumerate over object's slots.

Comment 8

[Samuel Sidler (old account; do not CC)]

Ben: Do you have time to backport this or do we need to find an owner for this 1.9.0.16 blocker?

Igor: Can you own this?

Code freeze is scheduled for November 10 at 11:59pm PDT.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I would think someone more familiar with jseng should look here...

Comment 10

[Igor Bukanov]

Comment on attachment 330564 [details] [diff] [review]
Patch, v2

After file name change the patch applies as-is to 1.9.0.

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 330564 [details] [diff] [review]
Patch, v2

Approved for 1.9.0.16, a=dveditz for release-drivers

Comment 12

[Igor Bukanov]

landed to 1.9.0 branch:

Checking in jslock.c;
/cvsroot/mozilla/js/src/jslock.c,v  <--  jslock.c
new revision: 3.85; previous revision: 3.84

Comment 13

[Al Billings [:abillings - ex-MoCo]]

Is there anything specific that can be done here to verify this for 1.9.0.16?

Comment 14

[Igor Bukanov]

(In reply to comment #13)
> Is there anything specific that can be done here to verify this for 1.9.0.16?

I am not aware f browser-related tests. On the other hand a test for js shell should be feasible. But I do not have time this week to create one.