Thunderbird crash on startup [@js_HashString()]

RESOLVED FIXED

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Wladimir Palant, Assigned: Brian Crowder)

Tracking

({fixed1.8.0.12})

1.8 Branch
fixed1.8.0.12
Points:
---
Bug Flags:
wanted1.8.1.x -
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:moderate?])

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

10 years ago
I got a bug report about Thunderbird crashing on startup under certain conditions. These conditions are: Mac OS X 10.3.9, Thunderbird 1.5.0.9, Adblock Plus 0.7.2.4, FlashGot 0.5.97.03 and a specific patterns.ini that will be loaded by Adblock Plus on startup. The crash is stable:

TB29095135Z
TB29095158G
TB29095216X
TB29095954K
TB29096078K

Here is the stack from the talkback reports:

js_HashString()  [mozilla/js/src/jsstr.c, line 2825]
js_AtomizeString()  [mozilla/js/src/jsatom.c, line 651]
fun_xdrObject()  [mozilla/js/src/jsfun.c, line 1350]
js_XDRObject()  [mozilla/js/src/jsobj.c, line 4323]
JS_XDRValue()  [mozilla/js/src/jsxdrapi.c, line 544]
XDRAtomListElement()  [mozilla/js/src/jsscript.c, line 351]
XDRAtomMap()  [mozilla/js/src/jsscript.c, line 385]
js_XDRScript()  [mozilla/js/src/jsscript.c, line 519]
JS_XDRScript()  [mozilla/js/src/jsxdrapi.c, line 579]
nsXULPrototypeScript::Deserialize()  [mozilla/content/xul/content/src/nsXULElement.cpp, li]
nsXULPrototypeScript::DeserializeOutOfLine()  [mozilla/content/xul/content/src/nsXULElemen]
nsXULPrototypeElement::Deserialize()  [mozilla/content/xul/content/src/nsXULElement.cpp, l]
nsXULPrototypeDocument::Read()  [mozilla/content/xul/document/src/nsXULPrototypeDocument.]
nsXULPrototypeCache::GetPrototype()  [mozilla/content/xul/document/src/nsXULPrototypeCach]
nsChromeProtocolHandler::NewChannel()  [mozilla/chrome/src/nsChromeProtocolHandler.cpp, line 842]
nsIOService::NewChannelFromURI()
NS_NewChannel()  [mozilla/mailnews/mime/src/mimecth.cpp, line 172]
nsDocShell::DoURILoad()
nsDocShell::InternalLoad()
nsDocShell::LoadURI()
nsDocShell::LoadURI()
nsWebShellWindow::Initialize()
nsAppShellService::JustCreateTopWindow()
nsAppShellService::CreateHiddenWindow()
nsAppStartup::CreateHiddenWindow()
XRE_main()  [mozilla/toolkit/xre/nsAppRunner.cpp, line 2271]
_start()   start()

Not sure this crash can be reproduced but I will attach the patterns.ini file I was sent (should be placed into the adblockplus directory in the profile). Since this seems to depend on the objects that Adblock Plus creates and since js_HashString clearly gets a wrong memory location from somewhere I am marking this security sensitive just to be sure.
(Reporter)

Comment 1

10 years ago
Created attachment 254402 [details]
patterns.ini file required for the crash (gzipped)
(Reporter)

Updated

10 years ago
Attachment #254402 - Attachment mime type: text/plain → application/octet-stream
Flags: blocking1.9?
(Reporter)

Comment 2

10 years ago
Tried to reproduce this crash, didn't work - neither on Mac OS X nor on Windows. Anybody able to make an educated guess from the stack trace?

Comment 3

10 years ago
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/jsfun.c&mark=1224,1228,1297,1349&rev=THUNDERBIRD_1_5_0_9_RELEASE#1224

personally, i'd be asking what protects atomstr from dying, especially given that, i think propname could evict it.
propname is initialized from JS_GetStringBytes, which deflates a string into the deflated string cache, but does not allocate a GC-thing. So atomstr still should be hanging by a thin reed. But it's a long way from where atomstr is created to where it's atomized, so yeah: JS_PUSH_TEMP_ROOT_STRING should be used.

Cc'ing folks who should fight to take this bug.

/be
(Assignee)

Comment 5

10 years ago
I will take this one as soon as I am done with the array bug.
Assignee: general → crowder
OS: Mac OS X → All
Hardware: Macintosh → All
(Assignee)

Comment 6

10 years ago
Created attachment 256972 [details] [diff] [review]
add a root for atomstring

I don't have a working thunderbird build to test this with, so help would be appreciated.
Attachment #256972 - Flags: review?(brendan)
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
Comment on attachment 256972 [details] [diff] [review]
add a root for atomstring

Don't add string to JSTempValueUnion. Do consolidate tvr per C function, there's no need for two.

Igor should be reviewer of choice on such patches, so I don't delay response and cuz he's tvr meister.

/be
Attachment #256972 - Flags: review?(brendan) → review-
(Assignee)

Comment 8

10 years ago
Created attachment 259144 [details] [diff] [review]
removing unnecessary tvr

Brendan:  I added JSString to the JSTempValueUnion structure because the macro rooting macro here which was ported from the trunk relies on it, and the macro/type-safe union seems cleaner than using a cast or void *.  Hopefully you or Igor will correct me if that is still not correct.  I have added the JSString * to the union in the same place as it is added on the trunk.
Attachment #256972 - Attachment is obsolete: true
Attachment #259144 - Flags: review?(igor)

Comment 9

10 years ago
Comment on attachment 259144 [details] [diff] [review]
removing unnecessary tvr


>     /* From here on, control flow must flow through label out. */
>     JS_PUSH_SINGLE_TEMP_ROOT(cx, OBJECT_TO_JSVAL(fun->object), &tvr);
>+    JS_PUSH_TEMP_ROOT_STRING(cx, atomstr, &tvr);
>     ok = JS_TRUE;

tvr stores the linked list next link so one can not use 2 pushes per one tvr. You need either 2 JSTempValueRooter instances here or better yet one tvr and a jsval[2] array together with code that populates it like in:

jsval roots[2];
...

roots[0] = OBJECT_TO_JSVAL(fun->object);
roots[1] = STRING_TO_JSVAL(atomstr);
JS_PUSH_TEMP_ROOT(cx, JS_ARRAY_LENGTH(roots), roots, &tvr);
Attachment #259144 - Flags: review?(igor) → review-
(Assignee)

Comment 10

10 years ago
Created attachment 259156 [details] [diff] [review]
jsval array instead of multiple tvrs

Thanks for the explanation, Igor, I started with two tvrs in my original patch; is there a reason one tvr with a jsval array is superior, or is that primarily a style issue?
Attachment #259144 - Attachment is obsolete: true
Attachment #259156 - Flags: review?(igor)

Comment 11

10 years ago
(In reply to comment #10)
> Created an attachment (id=259156) [details]
> jsval array instead of multiple tvrs
> 
> Thanks for the explanation, Igor, I started with two tvrs in my original patch;
> is there a reason one tvr with a jsval array is superior, or is that primarily
> a style issue?

tvr with jsval[2] just brings less code as the extra linking/unlinking of tvr from the JSContext list is avoided.

Comment 12

10 years ago
Comment on attachment 259156 [details] [diff] [review]
jsval array instead of multiple tvrs

>     /* From here on, control flow must flow through label out. */
>-    JS_PUSH_SINGLE_TEMP_ROOT(cx, OBJECT_TO_JSVAL(fun->object), &tvr);
>+    roots[0] = OBJECT_TO_JSVAL(fun->object);
>+    roots[1] = STRING_TO_JSVAL(atomstr);
>+    JS_PUSH_TEMP_ROOT(cx, sizeof(roots) / sizeof(roots[0]), roots, &tvr);
>     ok = JS_TRUE;
> 
>     if (!JS_XDRStringOrNull(xdr, &atomstr) ||

Sorry I did not see it the first time, but after decoding atomstr would contain a new string. It is unrooted as roots[1] contains unrelated value. So lets add a second tvr with JSString* member added and then use atomstrTvr.u.string, not atomstr in the rest of code.
Attachment #259156 - Flags: review?(igor) → review-
(Assignee)

Comment 13

10 years ago
Created attachment 259226 [details] [diff] [review]
two tvrs again, and modified string usage

I like this patch less (accessing the union elements is ugly), I'd almost rather keep the jsval array and just set the string after the XDR decode.  Thoughts?  Also, I could of course remove atomstr entirely, but again it looks uglier.
Attachment #259156 - Attachment is obsolete: true

Comment 14

10 years ago
Comment on attachment 259226 [details] [diff] [review]
two tvrs again, and modified string usage

Yes, this is ugly, but at least it works. I also agree that atomizing the string right after JS_XDRStringOrNull and storing it as fun->atom ASAP would also work.
Attachment #259226 - Flags: review+
(Assignee)

Comment 15

10 years ago
(In reply to comment #14)
> (From update of attachment 259226 [details] [diff] [review])
> I also agree that atomizing the string right after JS_XDRStringOrNull
> and storing it as fun->atom ASAP would also work. 

Do you mind explaining this part in more depth?

Comment 16

10 years ago
(In reply to comment #15)
> (In reply to comment #14)
> > (From update of attachment 259226 [details] [diff] [review] [details])
> > I also agree that atomizing the string right after JS_XDRStringOrNull
> > and storing it as fun->atom ASAP would also work. 
> 
> Do you mind explaining this part in more depth?

The idea is to move 

    if (xdr->mode == JSXDR_DECODE) {
...
        if (atomstr) {
             /* XXX only if this was a top-level function! */
             fun->atom = js_AtomizeString(cx, atomstr, 0);
             if (!fun->atom)
                 goto bad;
         }
   }

right after JS_XDRStringOrNull(xdr, &atomstr). It this way there is no need to protect atomstr against GC.
Please avoid atomstrTvr type names -- too many consonants in a row. atomtvr would be nicer, but comment 16 has the better idea.

/be
(Assignee)

Comment 18

10 years ago
Created attachment 259421 [details] [diff] [review]
atomize the string earlier

I like this version best so far.  And, I'm pretty sure that moving the majority of this chunk up is safe.
Attachment #259421 - Flags: review?(igor)

Updated

10 years ago
Attachment #259421 - Flags: review?(igor) → review+
(Assignee)

Comment 19

10 years ago
The trunk code is way different, so this bug shouldn't be a 1.9 blocker.

Gonna try to test this out with tbird now.
Flags: blocking1.9?
(Assignee)

Comment 20

10 years ago
Comment on attachment 259421 [details] [diff] [review]
atomize the string earlier

I believe this bug is either 1.8.0-only, or would present very differently elsewhere.
Attachment #259421 - Flags: approval1.8.0.12?
Comment on attachment 259421 [details] [diff] [review]
atomize the string earlier

approved for 1.8.0.12, a=dveditz for release-drivers
Attachment #259421 - Flags: approval1.8.0.12? → approval1.8.0.12+
(Assignee)

Comment 22

10 years ago
jsfun.c: 3.117.2.7.2.13
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Flags: in-testsuite-
That's fixed1.8.0.12, right?
Flags: wanted1.8.1.x-
Keywords: fixed1.8.0.12
(Assignee)

Comment 24

10 years ago
Yes, though in communicating with an external embedder, I'm thinking I may have just moved this or discovered another bug nearby.  I will post more when I have had time to work it out with them.
guessing sg:moderate, but could be sg:critical if there's a reliable web-content testcase that doesn't require particular extensions.
Whiteboard: [sg:moderate?]
Group: security
You need to log in before you can comment on or make changes to this bug.