TM: Unsafe loop in js_ClearNative

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
8 years ago
3 years ago

People

(Reporter: dmandelin, Assigned: Igor Bukanov)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey, [sg:critical], [critsmash:patch])

(Reporter)

Description

8 years ago
js_ClearNative has this code:

    JSScope *scope;
    uint32 i, n;

    /*
     * Clear our scope and the property cache of all obj's properties only if
     * obj owns the scope (i.e., not if obj is sharing another object's scope).
     * NB: we do not clear any reserved slots lying below JSSLOT_FREE(clasp).
     */
    JS_LOCK_OBJ(cx, obj);
    scope = obj->scope();
    if (!scope->isSharedEmpty()) {
        /* Now that we're done using scope->lastProp/table, clear scope. */
        scope->clear(cx);

        /* Clear slot values and reset freeslot so we're consistent. */
        i = obj->numSlots();
        n = JSSLOT_FREE(obj->getClass());
        while (--i >= n)
            obj->setSlot(i, UndefinedValue());
        scope->freeslot = n;

I believe that |n| can be 0 now, so using unsigned ints here can underflow. I fixed this in JM as bug 583402.

Comment 1

8 years ago
igor, could you take a look at 

http://hg.mozilla.org/projects/jaegermonkey/rev/fbbb0c6655c9

we think this is what hit bug 579957

Updated

8 years ago
Assignee: general → igor
(Reporter)

Comment 2

8 years ago
(In reply to comment #1)
> igor, could you take a look at 
> 
> http://hg.mozilla.org/projects/jaegermonkey/rev/fbbb0c6655c9
> 
> we think this is what hit bug 579957

Please note there is a bug in my original fix. I used the wrong value for the new value of scope->freeslot. I pushed a followup here:

http://hg.mozilla.org/projects/jaegermonkey/rev/8a3800153866
(Assignee)

Comment 3

8 years ago
(In reply to comment #1)
> igor, could you take a look at 
> 
> http://hg.mozilla.org/projects/jaegermonkey/rev/fbbb0c6655c9
> 
> we think this is what hit bug 579957

Right, this indeed the case.
(Assignee)

Comment 4

8 years ago
http://hg.mozilla.org/tracemonkey/rev/8d4eaefc5894 - landed to TM.

This is the JM fix with a nit fixed to use the original uint32, not int, for indexes as both JSCLASS_FREE(clasp) and obj->numSlots() returns uint32.
(Assignee)

Updated

8 years ago
Blocks: 579957

Updated

8 years ago
Whiteboard: fixed-in-tracemonkey, [sg:critical]

Updated

8 years ago
Whiteboard: fixed-in-tracemonkey, [sg:critical] → fixed-in-tracemonkey, [sg:critical], [critsmash:patch]
(Reporter)

Updated

8 years ago
Duplicate of this bug: 583421

Comment 6

8 years ago
http://hg.mozilla.org/mozilla-central/rev/8d4eaefc5894
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Group: core-security
You need to log in before you can comment on or make changes to this bug.