28982 - JS_ClearScope can cause later wild pointer writes

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Ninoschka Baca]

Build 2000-02-23-08M14: NT4, Linux 6.0
Haven't tried the Mac build yet.

Overview: A crash occurs after selecting the Server panel, Copies & Folders and 
selecting OK in the Account Settings window.

Steps to reproduce:
1. Open the Account Settings window
2. Select the Server panel for an account
3. Select the Copies & Folders panel for the same account
4. Select the OK button

Actual Results: A crash occurs
Expected Results: A crash should not occur

Additional Information:
- Linux: Build 2/22, it crashes after selecting the panels multiple times.
- Mac: Build 2/22, using same scenario does not crash.

Comment 1

[Ninoschka Baca]

Incident# 5796477

 Call Stack:    (Signature = JS_HashTableDestroy a468e562)     
   JS_HashTableDestroy                                         
[d:\builds\seamonkey\mozilla\js\src\jshash.c, line 148]   
   js_hash_scope_clear                                         
[d:\builds\seamonkey\mozilla\js\src\jsscope.c, line 221]  
   js_DestroyScope                                      
[d:\builds\seamonkey\mozilla\js\src\jsscope.c, line 425]
   js_DestroyObjectMap                                        
[d:\builds\seamonkey\mozilla\js\src\jsobj.c, line 1193]  
   js_DropObjectMap                                         
[d:\builds\seamonkey\mozilla\js\src\jsobj.c, line 1210]  
   js_FinalizeObject                                
[d:\builds\seamonkey\mozilla\js\src\jsobj.c, line 1392]
   js_GC                                         
[d:\builds\seamonkey\mozilla\js\src\jsgc.c, line 899]
   js_ForceGC                                         
[d:\builds\seamonkey\mozilla\js\src\jsgc.c, line 679]
   JS_GC                                      
[d:\builds\seamonkey\mozilla\js\src\jsapi.c, line 1167]
   nsJSContext::GC                                       
[d:\builds\seamonkey\mozilla\dom\src\base\nsJSEnvironment.cpp, line 746]
   nsQueryInterface::operator()                                       
[d:\builds\seamonkey\mozilla\xpcom\base\nsCOMPtr.cpp, line 37]
MSVCRT.dll + 0x12af (0x780012af) 
DocumentViewerImpl::~DocumentViewerImpl                                        
[d:\builds\seamonkey\mozilla\layout\base\src\nsDocumentViewer.cpp, line 364] 
   DocumentViewerImpl::`scalar deleting
   destructor'                                           
   DocumentViewerImpl::Release                                         
[d:\builds\seamonkey\mozilla\layout\base\src\nsDocumentViewer.cpp, line 308]    
   nsCOMPtr_base::assign_with_AddRef                                          
[d:\builds\seamonkey\mozilla\xpcom\base\nsCOMPtr.cpp, line 59]    
   nsWebShell::CreateViewer                                          
[d:\builds\seamonkey\mozilla\webshell\src\nsWebShell.cpp, line 1885]   
   nsWebShell::CreateViewer                                          
[d:\builds\seamonkey\mozilla\webshell\src\nsWebShell.cpp, line 1885]    
   nsWebShell::DoContent                                         
[d:\builds\seamonkey\mozilla\webshell\src\nsWebShell.cpp, line 1779]   
   nsDocumentOpenInfo::DispatchContent              
[d:\builds\seamonkey\mozilla\uriloader\base\nsURILoader.cpp, line 388]
   nsDocumentOpenInfo::OnStartRequest                 
[d:\builds\seamonkey\mozilla\uriloader\base\nsURILoader.cpp, line 249]
   nsResChannel::OnStartRequest            
[d:\builds\seamonkey\mozilla\netwerk\protocol\res\src\nsResChannel.cpp, line 
574]
   nsFileChannel::OnStartRequest           
[d:\builds\seamonkey\mozilla\netwerk\protocol\file\src\nsFileChannel.cpp, line 
438]
   nsOnStartRequestEvent::HandleEvent            
[d:\builds\seamonkey\mozilla\netwerk\base\src\nsAsyncStreamListener.cpp, line 
203]
   nsStreamListenerEvent::HandlePLEvent                                        
[d:\builds\seamonkey\mozilla\netwerk\base\src\nsAsyncStreamListener.cpp, line 
98]
   PL_HandleEvent                             
[d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c, line 527]
   _md_EventReceiverProc                                   
[d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c, line 977]
     USER32.dll + 0x1820 (0x77e71820) 
                                           

Comment 2

[Ninoschka Baca]

The crash doesn't occur using Mac build 2000-02-23-12M14. So the problem occurs 
on Linux and NT4.

Comment 3

[sol]

I think this should be in for beta1. Any Windows or Linux user who reviews their 
mail account settings (by clicking Server then Copies & folders panels) will hit 
this bug, and the results are disastrous - crash.

Reasonably high frequency of occurence and very bad result.

Comment 4

[Ninoschka Baca]

Attachment: Redirecting the console output. This file should show what the console would have displayed.

Comment 5

[leger]

Putting on PDT+ radar for beta.

Comment 6

[Alec Flett]

console output suggests JS is very broken or confused. reassign to jband

Comment 7

[John Bandhauer]

I'm hot on the trail of this. In a debug build I see it manifesting as a 
debug heap check that finds 0x80000001 written on memory that the heap does not 
expect anyone to write on. I see this repeatably, but in different locations. 
That value corresponds to JSVAL_VOID. With a Purify build I was able to narrow 
this to a write in jsobj.c line 1448. A conditional breakpoint (when it writes 
to the memory the heap manager set to 0xfeeefeee) got me live into the debugger.

If is doing "obj->slots[slot] = JSVAL_VOID;" with 'slot' being 0xc8 but that 
object's map->nslots is only 0xa0. The sprop in question has setter/getter for 
Set/GetWindowProperty. The (nice long) comment in the code implies that this is 
a cleanup that can be happening long after the fact of property iteration. Since 
we recycle windows and refill them with new props I gotta look into that. This 
code could be more generally screwing up in non-window cases too.

It is late and I have to go home. I'll jump on this in the morning and figure it 
out.

Comment 8

[John Bandhauer]

*******************************************************************************
*** I'm hoping that one of the JS engine gurus will read, think, and comment***

OK. This is a core JS engine bug.

I've got a fairly good handle on when is going wrong, but I'm not completely 
sure how best to fix it.

The core of the problem is that in js_SetProperty we assume that after calling 
SPROP_SET we can call js_DropScopeProperty with the same scope and (esp.) sprop 
that we gathered before calling SPROP_SET. The problem is that a native setter 
(or even a JS setter?) can alter the property set of the object enough for the 
info in sprops (and scope?) to be incorrect.

The specific case in this bug is that the JS code causing the js_SetProperty 
call is an assignment to 'window.location'. This forces a complete clearing 
of the window object and relaoding with a new documents etc. So, the obj is a 
window and the native setter ends up clearing the object's scope and doing a 
bunch of JS_DefineProperty and JS_DefineFunction calls on the object. More often 
that not nothing *noticably* bad happens. But the code in js_AllocSlot has a 
heuristic regarding the map->nslots and map->freeslots that will sometimes cause 
it to use realloc to shrink the object's map in place. In this case after the 
call to SPROP_SET is done and we then do js_DropScopeProperty as it decrements 
the recount it discovers that the (old) property 'window.location' that was 
being set has no other referers (i.e. --sprop->nrefs == 0) so it calls 
js_DestroyScopeProperty which calls js_FreeSlot which does "obj->slots[slot] = 
JSVAL_VOID" where 'slot' is now a number larger than the length of our slot 
array!

So. Some questions...

- Is this a bad assumption in js_SetProperty or *only* bad code in 
JS_ClearScope?
- Do operations other than js_SetProperty make the same mistake?
- Clearly the target object has been morphed enough that sprop is out of sync. 
Can the object be morphed such that scope is no longer the right scope? - It was 
not a mutated scope before the call to SPROP_SET but is mutated after?

I started out by thinking the this code in js_SetProperty was at fault. But now 
I'm thinking that its assumptions are safe and that JS_ClearScope is the real 
problem.

What to do?

First off an JS_ASSERT in js_FreeSlot is in order...
>   JS_ASSERT(slot < obj->map->freeslot);
    obj->slots[slot] = JSVAL_VOID;

Of course, that is not enough. Even a non-assert check and return here is not 
enough because even though this might still be a valid slot, it might have been 
recycled and used by some other property at this point. This might be 
happening a lot! Right?

It looks to me like when simply deleting a property (say, as in a setter that 
deletes its own self from the object) the system does not clear the slot and 
potentially decrement freeslots until the sprop refcount goes to zero. [I'd like 
to further verify this!]. If that is so then the real culprit is JS_ClearScope 
which calls scope->ops->clear just fine, but then *unconditionally* resets 
map->freeslot to the 'initial' value using JSSLOT_FREE. Clearly this is unsafe 
since there can be outstanding refs on some of the slots with numbers higher 
than the new value of freeslot.

So, again, what to do? I'm thinking that the resetting of freeslot in 
JS_ClearScope is the one and only problem (but I'm not certain). If we just left 
freeslots alone there then the slot array of objects like window could grow 
very large and fragmented. I think that as part of the scope clearing process we 
ought to be figuring out the highest "still in use" slot and then setting 
freeslots accordingly. There is no mechanism in place to do that now. We could 
make the 'clear' scope op(s) do that.

Does this make sense? 

I'm going to code up a simpler native test case since the means of reproduction 
in the browser are sooo painful and unpredicatable. I'll do more experimentation 
and code up a proposed fix.

Comments and insights welcome!

Comment 9

[John Bandhauer]

Attachment: patch to xpcshell to allow call to JS_ClearScope from JS

Comment 10

[John Bandhauer]

Attachment: test.js showing overwrite in slot after clear in setprop

Comment 11

[John Bandhauer]

Attachment: test2.js showing no overwrite after delete in setprop

Comment 12

[John Bandhauer]

My attached files above include:
1) A patch to xpcshell to give it a global 'clear' method that will do a 
JS_ClearScope on an object from a call in JavaScript.
2) A testcase that uses 'clear' in a setter. In the setter it clears the scope 
of 'this' and then adds two properties to 'this'. The output...
undefined
undefined
2
bar
baz
... shows that the value of the first property added after the clear is getting 
overwritten (presumably in js_FreeSlot after the setter returns - I'll verify).
3) test2.js uses 'delete' in the setter to delete the setter from 'this' but 
otherwise does the same as test.js. It outputs...
undefined
1
2
bar
baz
...which is what we would also expect to see from test.js.

Comment 13

[John Bandhauer]

Still talking to myself...

Now I'm thinking that not fully resetting freeslots in JS_ClearScope would cause 
plenty of slot fragmentation and waste. Instead, I'm thinking that JS_ClearScope 
should set (almost) all the slots to JSVAL_VOID itself and that the scope clear 
ops should set a flag in the sprops that tell them that they should *not* free 
their slot upon destruction. I'll investigate the implications of this strategy.

Comment 14

[John Bandhauer]

Changing the bug summary from...
   "Crash after select Server panel, then Copies panel"
...to...
   "JS_ClearScope can cause later wild pointer writes"

Changing 'Product' from "MailNews" to "Browser" and 'Components' from "Account 
Manager" to "JavaScript Engine"

Comment 15

[Brendan Eich [:brendan]]

So reference counting and deferred slot freeing make js_SetProperty safe against 
delete or clear-scope operations from within setters, as you surmised.  You're 
on the right track in attacking JS_ClearScope -- the XXX comment there shows up 
bad layering and kludging from the old days, when it was never meant to be 
called inside a setter (MozillaClassic cleared in a callback from the routine 
that started a URL load due to window.location being set, IIRC).

The whole freeslot/nslots affair is a poor man's first-fit bitmap allocator, 
geared towared LIFO allocation of slots (see js_FreeSlot, where freeslot is 
lowered by one if the last slot is being freed).  We could do better, but now 
may not be the time to get fancy.

Rather than add an sprop flag saying "don't free your slot", you might just set 
all outstanding sprop->slot = 0xffffffff or 0xdeadbeef, and add code testing for 
that slot number to govern the js_FreeSlot call in js_DestroyScopeProperty.  I 
like that better, and it can be done inside jsscope.c.

/be

Comment 16

[John Bandhauer]

Brendan, Thanks for taking a look. I like the sprop->slot = 0xffffffff idea. 
However, I do think it's important to expose this in jsscope.h so that outside 
users of the sprop can also detect that they should not trust the sprop->slot. 
Something like..

#define SPROP_INVALID_SLOT 0xffffffff
#define SPROP_IS_VALID_SLOT(_s) ((_s) != SPROP_INVALID_SLOT)

I'll write some code and attach diffs today.

Comment 17

[John Bandhauer]

Attachment: proposed fix. Needs more testing

Comment 18

[John Bandhauer]

Attached my proposed fix. I'm not sure if it is correct to set 
SPROP_INVALID_SLOT everytime js_DropScopeProperty reveals and 
outstanding reference on the sprops in js_free_symbol. Are there cases where 
outstanding references at this point could still need the slot and 
js_free_symbol is being called for something other than as a result of 
JS_ClearScope?

Even if this is OK, it still needs more testing.

Comment 19

[Alec Flett]

I just applied the patch to my tree. I'll try running with it today

Comment 20

[Brendan Eich [:brendan]]

It's incorrect to set SPROP_INVALID_SLOT in js_free_symbol after dropping the 
symbol's reference on the property: there could be other symbols aliasing that 
property, and using its slot.  Even if there aren't aliases outstanding, there 
could be a watchpoint set on the property (see jsobj.c:js_SetProperty, #ifdef 
JS_HAS_OBJ_WATCHPOINT).

It seems to me you need only set SPROP_INVALID_SLOT in the two jsscope.c-homed 
js_*_scope_clear() implementations, using JS_HashTableEnumerateEntries for the 
_hash_ flavor, and inserting code into the loop over symbols in the _list_ case 
to zap the symbol's property's slot (whew).

/be

Comment 21

[John Bandhauer]

Got it. That was my backup plan. I just was having trouble figuring out if 
the side effects of what i did mattered or not. I'll do as you suggest. Thanks.

Comment 22

[John Bandhauer]

Attachment: a better fix

Comment 23

[John Bandhauer]

I attached a better fix patch that does as brendan suggested and limits the 
places where it munges sprop->slot. The one case where I think this will have a 
new side effect is that other callers to ops->clear will no longer have the 
object's slot contents set to JSVAL_VOID if there are no other outstanding 
references to the sprop. But this seems like a non-problem to me. The only other 
caller I see is js_DestroyScope which seems to only be called by 
js_DestroyObjectMap which will likely be busy tearing down the object anyway. Do 
I have this right?

Comment 24

[Brendan Eich [:brendan]]

js_SetProperty does need a tweak after all, to handle the unlikely case of a 
property with a watchpoint whose owning symbol(s) are deleted or clear-scoped 
under the call to the setter, but whose reference count remains 1 due to the 
watchpoint:

     /* Relock obj until we are done with sprop. */
     JS_LOCK_OBJ(cx, obj);
 
     sprop = js_DropScopeProperty(cx, scope, sprop);
-    if (!sprop) {
+    if (!sprop || !SPROP_HAS_VALID_SLOT(sprop)) {
	 /* Lost a race with someone who deleted sprop. */
	 JS_UNLOCK_OBJ(cx, obj);
	 return JS_TRUE;
     }

(I just made up SPROP_HAS_VALID_SLOT(sprop) for conciseness over 
!SPROP_IS_VALID_SLOT(sprop->slot) -- add it to the patch if it does economize 
for all calls to SPROP_IS_VALID_SLOT, and if you like. ;-).

Now for js_GetProperty's call to a getter that deletes or clears: it turns out I 
blew it, and failed to fix bug 14462 completely.  Here's a pseudo-patch for 
jsobj.c to the body of js_GetProperty:

{
    JSObject *obj2;
    JSScopeProperty *sprop;
+    JSScope *scope;
    jsint slot;

    if (!js_LookupProperty(cx, obj, id, &obj2, (JSProperty **)&sprop))
        return JS_FALSE;
    if (!sprop) {
        /*
         * Handle old bug that treated empty string as zero index.
         * Also convert string indices to numbers if applicable.
         */
        CHECK_FOR_FUNNY_INDEX(id);

#if JS_BUG_NULL_INDEX_PROPS
        /* Indexed properties defaulted to null in old versions. */
        *vp = (JSVAL_IS_INT(id) && JSVAL_TO_INT(id) >= 0)
              ? JSVAL_NULL
              : JSVAL_VOID;
#else
        *vp = JSVAL_VOID;
#endif

        return OBJ_GET_CLASS(cx, obj)->getProperty(cx, obj, js_IdToValue(id),
                             vp);
    }

    if (!OBJ_IS_NATIVE(obj2)) {
        OBJ_DROP_PROPERTY(cx, obj2, (JSProperty *)sprop);
        return OBJ_GET_PROPERTY(cx, obj2, id, vp);
    }

    /* Unlock obj2 before calling getter, relock after to avoid deadlock. */
+    scope = OBJ_SCOPE(obj2);
    slot = sprop->slot;
    *vp = LOCKED_OBJ_GET_SLOT(obj2, slot);
+#ifndef JS_THREADSAFE
+    JS_ATOMIC_ADDREF(&sprop->nrefs, 1);
+#endif
    JS_UNLOCK_SCOPE(cx, scope);
    if (!SPROP_GET(cx, sprop, obj, obj2, vp)) {
-        JS_LOCK_OBJ(cx, obj2);
-        OBJ_DROP_PROPERTY(cx, obj2, (JSProperty *)sprop);
+        JS_LOCK_OBJ_VOID(cx, obj2, js_DropScopeProperty(cx, scope, sprop));
        return JS_FALSE;
    }
-    JS_LOCK_OBJ(cx, obj2);
-    LOCKED_OBJ_SET_SLOT(obj2, slot, *vp);
-    PROPERTY_CACHE_FILL(cx, &cx->runtime->propertyCache, obj2, id,
-                        (JSProperty *)sprop);
-    OBJ_DROP_PROPERTY(cx, obj2, (JSProperty *)sprop);
+    JS_LOCK_SCOPE(cx, scope);
+    sprop = js_DropScopeProperty(cx, scope, sprop);
+    if (sprop && SPROP_HAS_VALID_SLOT(sprop)) {
+        LOCKED_OBJ_SET_SLOT(obj2, slot, *vp);
+        PROPERTY_CACHE_FILL(cx, &cx->runtime->propertyCache, obj2, id,
+                            (JSProperty *)sprop);
+    }
+    JS_UNLOCK_SCOPE(cx, scope);
    return JS_TRUE;
}

Here, scope holds obj2->map, which cannot change (OBJ_SCOPE(obj2)->object==obj2 
by definition; see js_LookupProperty).  This leads to more optimal locking via 
JS_LOCK_SCOPE/JS_UNLOCK_SCOPE, except for the early return where I use the 
convenience macro JS_LOCK_OBJ_VOID to evaluate a void expression within a lock 
-- who cares about that case costing more by going through js_LockObj.

Also remember that OBJ_DROP_PROPERTY is a no-op for !JS_THREADSAFE, but both a 
js_DropScopeProperty and an unlock-obj's-scope op for the JS_THREADSAFE case.

N.B. that the holding and dropping of the property's reference count around the 
getter call-out (SPROP_GET) is not conditioned by #ifdef JS_THREADSAFE 
(wherefore the odd #ifndef JS_THREADSAFE to atomically increment -- this handles 
the non-threadsafe path where js_LookupProperty didn't add a ref -- maybe it'd 
be better if js_LookupProperty always held a ref for its caller when passed a 
non-null JSProperty* by reference, but it doesn't, cuz all this code was 
originally thread-unsafe only four years ago, and some unthreaded embedders 
still win a little by avoiding the holding and dropping code altogether via 
the #ifdefs and #ifdef'd macros).

Note that, as commented with XXXes, the addProperty and delProperty JSClass ops, 
called by js_DefineProperty/js_SetProperty and js_DeleteProperty respectively, 
cannot delete or clear properties, or in fact do anything that might lock 
another JS object, for fear of deadlock.  Works/broken as designed, and I don't 
think this is a hardship or current hazard.

/be

Comment 25

[Brendan Eich [:brendan]]

jband: right, js_DestroyScope only happens during a GC, and never during a 
getter or setter call-out (because even if a getter or setter called JS_GC, the 
object and scope in question would be protected by a GC root or strong link in 
the GC live-thing graph).

/be

Comment 26

[John Bandhauer]

Attachment: an even better fix

Comment 27

[John Bandhauer]

The patch above merges the changes from brendan (which I 95% understand). I 
think I got them right. I also threw a OBJ_CHECK_SLOT into js_FreeSlot to do an 
assert. Are we getting closer to something that passes the tests and someone 
might want to call reviewed?

Comment 28

[John Bandhauer]

I ran the JS test suite on NT (908 tests excluding the liveconnect tests) and it 
showed no failures.

Comment 29

[Brendan Eich [:brendan]]

Looks good, just one nit I botched the diff -u fakery for in my pseudo-patch -- 
in js_GetProperty:

     /* Unlock obj2 before calling getter, relock after to avoid deadlock. */
+    scope = OBJ_SCOPE(obj2);
     slot = sprop->slot;
     *vp = LOCKED_OBJ_GET_SLOT(obj2, slot);
+#ifndef JS_THREADSAFE
+    JS_ATOMIC_ADDREF(&sprop->nrefs, 1);
+#endif
     JS_UNLOCK_OBJ(cx, obj2);

Since you've loaded scope from OBJ_SCOPE(obj2), you might as well simplify that 
last line into

    JS_UNLOCK_SCOPE(cx, scope);

and cut out the middleman (js_UnlockObj calls js_UnlockScope if JS_THREADSAFE 
is defined).  If everyone's happy, r=[e-mail] and get this patch in. 
Thanks,

/be

Comment 30

[John Bandhauer]

Attachment: the best fix yet!

Comment 31

[John Bandhauer]

Fix checked in. Thanks Brendan!

Comment 32

[Ninoschka Baca]

Setting Target Milestone to M14 so it appears in my query.

Comment 33

[Ninoschka Baca]

Build 2000-02-28-08M15: NT4, Linux 6.0, Mac 8.5.1
Verified Fixed. Thanks!

Comment 34

[Ninoschka Baca]

Build 2000-02-28-08M15: NT4
Reopening because a crash is occuring on NT.

This build crashes after selecting the "Copies and Folders" panel a second time.
It happens with a POP or News account. 

Steps to reproduce:
1. Open Mail using a POP or News account
2. Open the Account Settings dialog
3. Select the Server Panel
4. Select the Copies Panel
5. Select the Server Panel
6. Select the Copies Panel
7. Select the Server Panel 

Actual Results: After selecting the Server Panel the right panel does not 
change. It continues to display "Copies and Folders" information. If the "OK" or 
"Cancel" button is selected then it crashes.
Expected Results: It should not crash.

Additional Information:
- NT4, 2/28 build: It doesn't crash when selecting panels for an IMAP account.
- Linux, 2/28 build: Is ok, no crash.
- Mac, 2/28 build: Is ok, no crash

Comment 35

[Alec Flett]

another stack trace could help us determine if it's the same crash or not

Comment 36

[John Bandhauer]

nbaca, could you verify which NT build you're running? Note that the fix went in 
after the M14 branch was made. So, a 2/28 M14 build would not have the fix. I 
know you said "Build 2000-02-28-08M15: NT4". But I'm looking in the 
ftp.mozilla.org nightlys directory and seeing that... 
ftp://ftp.mozilla.org/pub/mozilla/nightly/2000-02-28-08-M15/
...does not contain an NT build. Did you *maybe* get your builds from the other 
platforms from there and grab the NT build from a later dir on the same day? If 
so it might be that the NT build was from an M14 instead of M15 diretory? 

I just grabbed...
ftp://ftp.mozilla.org/pub/mozilla/nightly/2000-02-28-09-M15/mozilla-win32.zip
...and it worksforme just fine. I clicked back and forth a lot. Opened and 
closed the dialog.

Please confirm. I really think that this is fixed.

Comment 37

[Ninoschka Baca]

Build 2000-02-29-08M15: NT4
Fixed.

It is working and there is no crash. I've been going to the following ftp site 
for my builds - ftp://sweetlou/products/client/seamonkey/windows/32bit/x86/.
I see a Windows build in the morning of 02/28 for M15 and I'm pretty sure that's 
the one I pulled. I thought if it was fixed on 02/26 that the 02/28 build would 
have been ok. Sorry for any misunderstanding.

Comment 38

[Ninoschka Baca]

Verified Fixed. Thanks again!