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

RESOLVED FIXED

Status

()

RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: bent.mozilla, Assigned: igor)

Tracking

({fixed1.9.0.16})

Trunk
fixed1.9.0.16
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.0.16 +
wanted1.9.0.x +
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:dos] null deref)

Attachments

(1 attachment, 1 obsolete attachment)

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.
Attachment #328296 - Attachment is patch: true
Attachment #328296 - Attachment mime type: application/octet-stream → text/plain
Attachment #328296 - Flags: review?(brendan)
Assignee: general → bent.mozilla
Version: unspecified → Trunk
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,
Created attachment 330564 [details] [diff] [review]
Patch, v2

Updated to address brendan's comments.
Attachment #328296 - Attachment is obsolete: true
Attachment #330564 - Flags: review?(brendan)
Attachment #328296 - Flags: review?(brendan)
Comment on attachment 330564 [details] [diff] [review]
Patch, v2

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

/be
Attachment #330564 - Flags: review?(brendan) → review+
Pushed to moz-central.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Flags: in-testsuite-
Flags: in-litmus-

Updated

9 years ago
Blocks: 357052

Comment 6

9 years ago
For reference, the relevant revision is http://hg.mozilla.org/mozilla-central/rev/5f878cdd1d5e
(Assignee)

Comment 7

9 years ago
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.
Flags: blocking1.9.0.15?
Flags: blocking1.9.0.15? → blocking1.9.0.16?
Whiteboard: [sg:dos] null deref
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.16?
Flags: blocking1.9.0.16+
Blocks: 506823
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.
I would think someone more familiar with jseng should look here...
(Assignee)

Comment 10

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

After file name change the patch applies as-is to 1.9.0.
Attachment #330564 - Flags: approval1.9.0.16?
(Assignee)

Updated

9 years ago
Assignee: bent.mozilla → igor
Comment on attachment 330564 [details] [diff] [review]
Patch, v2

Approved for 1.9.0.16, a=dveditz for release-drivers
Attachment #330564 - Flags: approval1.9.0.16? → approval1.9.0.16+
(Assignee)

Comment 12

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

Updated

9 years ago
Keywords: fixed1.9.0.16
Is there anything specific that can be done here to verify this for 1.9.0.16?
(Assignee)

Comment 14

9 years ago
(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.
You need to log in before you can comment on or make changes to this bug.