806291 - Use of uninitialised value of size 4 in js::Int32ToString

Status: RESOLVED FIXED

Description

[Gary Kwong [:gkw] [:nth10sd]]

(Locking s-s because I'm not sure how bad this is. Feel free to open up if it is not s-s.)

Credit goes to Ishikawa, Chiaki who found this in bug 803816 comment 34.

m-c rev seems to be a517f7ea5bef

https://hg.mozilla.org/mozilla-central/annotate/a517f7ea5bef/js/src/vm/String-inl.h#l325

https://hg.mozilla.org/mozilla-central/annotate/a517f7ea5bef/js/src/jsstr.cpp#l3428


==8291== Use of uninitialised value of size 4
==8291==    at 0x677ABF6: js::Int32ToString(JSContext*, int) (String-inl.h:325)
==8291==    by 0x67DABC5: js::ToStringSlow(JSContext*, JS::Value const&) (jsstr.cpp:3428)
==8291==    by 0x676CE7D: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsstr.h:138)
==8291==    by 0x676DAD6: js::RunScript(JSContext*, JS::Handle<JSScript*>, js::StackFrame*) (jsinterp.cpp:324)
==8291==    by 0x676E07D: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jsinterp.cpp:379)
==8291==    by 0x676E6CD: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::Value*) (jsinterp.h:109)
==8291==    by 0x66E6691: JS_CallFunctionValue (jsapi.cpp:5884)
==8291==    by 0x57D7220: nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) (XPCWrappedJSClass.cpp:1420)
==8291==    by 0x57CFA0F: nsXPCWrappedJS::CallMethod(unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) (XPCWrappedJS.cpp:580)
==8291==    by 0x609B088: PrepareAndDispatch (xptcstubs_gcc_x86_unix.cpp:60)
==8291==    by 0x5BDB790: nsMsgMailSession::OnItemIntPropertyChanged(nsIMsgFolder*, nsIAtom*, int, int) (nsMsgMailSession.cpp:132)
==8291==    by 0x5BA6C53: nsMsgDBFolder::NotifyIntPropertyChanged(nsIAtom*, int, int) (nsMsgDBFolder.cpp:4925)
==8291==    by 0x5DD5B36: nsMsgNewsFolder::SetSortOrder(int) (nsNewsFolder.cpp:1891)
==8291==    by 0x5DD8596: nsMsgNewsFolder::AddNewsgroup(nsACString_internal const&, nsACString_internal const&, nsIMsgFolder**) (nsNewsFolder.cpp:290)
==8291==    by 0x5DD6287: nsMsgNewsFolder::CreateSubfolder(nsAString_internal const&, nsIMsgWindow*) (nsNewsFolder.cpp:576)
==8291==    by 0x13: ???
==8291==    by 0x740072: ???
==8291==

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I think that this is probably a bug in Thunderbird. There are actually a few warnings related to this one in the log. Here's the most relevant:

==8291== Conditional jump or move depends on uninitialised value(s)
==8291==    at 0x677AAD8: js::Int32ToString(JSContext*, int) (jsnum.cpp:524)
==8291==    by 0x67DABC5: js::ToStringSlow(JSContext*, JS::Value const&) (jsstr.cpp:3428)
==8291==    by 0x676CE7D: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsstr.h:138)
==8291==    by 0x676DAD6: js::RunScript(JSContext*, JS::Handle<JSScript*>, js::StackFrame*) (jsinterp.cpp:324)
==8291==    by 0x676E07D: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jsinterp.cpp:379)
==8291==    by 0x676E6CD: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::Value*) (jsinterp.h:109)
==8291==    by 0x66E6691: JS_CallFunctionValue (jsapi.cpp:5884)
==8291==    by 0x57D7220: nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) (XPCWrappedJSClass.cpp:1420)
==8291==    by 0x57CFA0F: nsXPCWrappedJS::CallMethod(unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) (XPCWrappedJS.cpp:580)
==8291==    by 0x609B088: PrepareAndDispatch (xptcstubs_gcc_x86_unix.cpp:60)
==8291==    by 0x5BDB790: nsMsgMailSession::OnItemIntPropertyChanged(nsIMsgFolder*, nsIAtom*, int, int) (nsMsgMailSession.cpp:132)
==8291==    by 0x5BA6C53: nsMsgDBFolder::NotifyIntPropertyChanged(nsIAtom*, int, int) (nsMsgDBFolder.cpp:4925)
==8291==    by 0x5DD5B36: nsMsgNewsFolder::SetSortOrder(int) (nsNewsFolder.cpp:1891)
==8291==    by 0x5DD8596: nsMsgNewsFolder::AddNewsgroup(nsACString_internal const&, nsACString_internal const&, nsIMsgFolder**) (nsNewsFolder.cpp:290)
==8291==    by 0x5DD6287: nsMsgNewsFolder::CreateSubfolder(nsAString_internal const&, nsIMsgWindow*) (nsNewsFolder.cpp:576)
==8291==    by 0x13: ???
==8291==    by 0x740072: ???

This is pointing to a problem here:

520 JSFlatString *
521 js::Int32ToString(JSContext *cx, int32_t si)
522 {
523     uint32_t ui;
524     if (si >= 0) {  // <----
525         if (StaticStrings::hasInt(si))
526             return cx->runtime->staticStrings.getInt(si);

It's saying that si has an unknown value. Unfortunately, the stack doesn't give us enough information to say where this value is coming from. However, my suspicion is that it's coming from nsMsgMailSession::OnItemIntPropertyChanged, which is part of Thunderbird.

In any case, I don't think we're doing anything unsafe here. We're reading from an array, but we do a bounds check first, so at worst we'll just get some random output.

Comment 2

[Mark Banner (:standard8)]

So following up the stack, it appears that nsMsgNewsFolder::mSortOrder doesn't get initialised, hence the warning. The value is passed down and the aOldValue to nsMsgMailSession::OnItemIntPropertyChanged is the one that will be uninitialised, which I suspect could confuse a few things, but I doubt it'd be anything significant.

Ishikawa could you try defaulting that parameter to 0 or kNewsSortOffset (http://hg.mozilla.org/comm-central/annotate/b1b1fd6de66d/mailnews/news/src/nsNewsFolder.cpp#l288) and see if that silences the warning?

Comment 3

[ISHIKAWA, Chiaki]

Will do. Took a couple of days to sort out 32 bit to 64 bit change (and
I will still upload the 32 bit result.).

Comment 4

[ISHIKAWA, Chiaki]

I am not sure what to do now.
In comm-central/source/mailnews/news/src/nsNewsFolder.cpp#563
we have the following code.

563 NS_IMETHODIMP nsMsgNewsFolder::CreateSubfolder(const nsAString& newsgroupName,
564                                                nsIMsgWindow *msgWindow)
565 {
566   nsresult rv = NS_OK;
567   if (newsgroupName.IsEmpty())
568     return NS_ERROR_FAILURE;
569 
570   nsCOMPtr<nsIMsgFolder> child;
571   // Create an empty database for this mail folder, set its name from the user
572   nsCOMPtr<nsIMsgDatabase> newsDBFactory;
573   nsCOMPtr <nsIMsgDatabase> newsDB;
574 

Maybe after line 574, I can simply do
child-> mSortOrder = 0 (or 9000 as noted in the comment 3).

Is that what you would like me to try? 
(Given the stack trace that I thought this is where I should set mSortOrder.
Or maybe I need to re-define nsMsgNewFolder (?) so that mSortOrder
is set to 0 for example? (but I could not find its definition, maybe I did not try hard enough. Also, changing possibly a header file is not quite desirable.
Lots of re-compilation on a non-powerful machine.)

I am not sure where you would like me to put mSortOrder to 0 where (which line and what file) exactly.
Tips welcome.

Comment 5

[Mark Banner (:standard8)]

I think we could set it in the constructor with the other member variables - then it is always initialised to a value, regardless of where it is used.

As to its values, I don't think it really matters, we could 0 or 9000, I'd be happy with either.

Comment 6

[ISHIKAWA, Chiaki]

Created attachment 677624 [details] [diff] [review]
Patch to set initialization value to mSortOrder

Running thunderbird under valgrind for testing purposes.
Under 32 bit linux.

I applied the suggested initialization to mSortOrder as suggested.

The reference to Int32ToString in the warning disappeared, and no
longer to be seen.  One progress.

I am attaching the log on next post.


There are a few questions that bother me.

[?] jsbridge reacheability

When I tried testing thunderbird under 64bits linux, there were errors
about jsbridge can not be connected. I still can't figure out what is
wrong. It could be under my setup, valgrind runs slower under 64 bits
and so I see extra time out, or maybe jsbridge testing is never
intended to run under 64 bits.

During the compilation toward the end, I see the following message
lines (both under 32bits and 64bits linux). Can this be related?
 
Unpacking ./jsbridge
  Running setup.py egg_info for package from file:///TB-NEW/TB-3HG/objdir-tb3/mozilla/_tests/mozmill/resources/jsbridge

    warning: no files found matching '*' under directory 'jsbridge/xpi'

I searched for jsbridge and saw a few mentions of similar problems?
bug 675787
bug 640435

[?] -foreground switch
 
Also, either my wrapper function is incorrect or something:
I see warning lines.

Warning: unrecognized command line flag -foreground

(maybe there is a difference beteen testing of thunderbird and
firefox? Or my wrapper is picking up one too many argument, but
if so, then something is incorrect. The argv[off_by_one_upper_limi]
contains some valid data(?!)

Comment 7

[ISHIKAWA, Chiaki]

Created attachment 677626 [details]
Running TB mozmill test TB under valgrind (32bits linux)

Anyway, here is the log.
I attach the full compilation log just before the testing starts
to show this warning about jsbridge/xpi/.
[Also, I am not sure if I am correct or not but, when
I changed nsNewsFolder.cpp and compiled it locally under
MYMOZOBJ/mailnews/news/src/
to check the correctness of the patch, and then saw the
single file compiled and a library is recreated, I  invoked
the top-level make just to be sure the binaries are recreated.
However, no compilation or linking happened, and copying of
testing scripts etc seemed to happen.
There could be brokenness in the Makefile infrastructure
in thunderbird.

This is why I felt it necessary to invoke the full make after the patch
to nsNewsFolder.cpp after removing the MOZOBJ directory.]

Would it be OK to post to the non s-s original bugzilla if another bug mentioned is fixed? 
This log now has more symbols since I searched and installed several debug symbol files for used libraries (under 32bits Debian GNU/Linux) and is more informative in terms of stack trace.

TIA

Comment 8

[ISHIKAWA, Chiaki]

I forgot to mention.
There are a few timeouts.
Also, there are a few cases of seemingly valid unexpected errors that may need
investigation (is it possible that assigning 9000 to mSortOrder broke something)?
This is another reason I feel it might be a good idea to post the full log to the non s-s bugzilla entry.

TIA

Comment 9

[Mark Banner (:standard8)]

(In reply to ISHIKAWA, chiaki from comment #6)
> [?] jsbridge reacheability
> 
> When I tried testing thunderbird under 64bits linux, there were errors
> about jsbridge can not be connected. I still can't figure out what is
> wrong. It could be under my setup, valgrind runs slower under 64 bits
> and so I see extra time out, or maybe jsbridge testing is never
> intended to run under 64 bits.

I believe there is a timeout on jsbridge/Mozmill (maybe set in one of the scripts?), but that's not really a discussion that's appropriate for this bug.

> Also, either my wrapper function is incorrect or something:
> I see warning lines.
> 
> Warning: unrecognized command line flag -foreground

That's just a side effect of how that flag is handled by the core code. No need to worry about it, just ignore it.

(In reply to ISHIKAWA, chiaki from comment #8)
> I forgot to mention.
> There are a few timeouts.
> Also, there are a few cases of seemingly valid unexpected errors that may
> need
> investigation (is it possible that assigning 9000 to mSortOrder broke
> something)?

Some of the timeouts are known ones that we've got in bugzilla, other failures I suspect are either random, or due to your configuration (I think I saw that crashreporter wasn't included in the build). I didn't see anything that suggested that the mSortOrder change broke anything.

Comment 10

[Mark Banner (:standard8)]

(In reply to ISHIKAWA, chiaki from comment #7)
> Would it be OK to post to the non s-s original bugzilla if another bug
> mentioned is fixed? 

I don't know what our normal process is for bugs found with valgrind, cc'ing some people who might know.

Comment 11

[ISHIKAWA, Chiaki]

Thank you for the follow up.

https://bugzilla.mozilla.org/attachment.cgi?id=677624
Maybe someone can push the above patch/diff for review?

I have found out how to make the time out longer, and
select a relatively smaller set of test target (per directory basis).
See Bug 805752
https://bugzilla.mozilla.org/show_bug.cgi?id=805752#c5
and the full log for a particular test target uploaded there.

Thank you again for making the great software available.
My office work depends on TB to run properly these days!

Comment 12

[Gary Kwong [:gkw] [:nth10sd]]

Comment on attachment 677624 [details] [diff] [review]
Patch to set initialization value to mSortOrder

You can set the flag to review? and select a reviewer - in this case I've done this and asking Mark for review for you.

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 677624 [details] [diff] [review]
Patch to set initialization value to mSortOrder

Review of attachment 677624 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/news/src/nsNewsFolder.cpp
@@ +168,5 @@
>  nsMsgNewsFolder::nsMsgNewsFolder(void) :
>       mExpungedBytes(0), mGettingNews(false),
> +     mInitialized(false),
> +     m_downloadMessageForOfflineUse(false), m_downloadingMultipleMessages(false),
> +     mReadSet(nullptr), mSortOrder(9000)

Should be using kNewsSortOffset rather than 9000 here. r=me with this fixed - I'll update the patch and land it for you.

Comment 14

[Mark Banner (:standard8)]

(In reply to Bill McCloskey (:billm) from comment #1)
> It's saying that si has an unknown value. Unfortunately, the stack doesn't
> give us enough information to say where this value is coming from. However,
> my suspicion is that it's coming from
> nsMsgMailSession::OnItemIntPropertyChanged, which is part of Thunderbird.
> 
> In any case, I don't think we're doing anything unsafe here. We're reading
> from an array, but we do a bounds check first, so at worst we'll just get
> some random output.

I'm in agreement with this assessment.

Comment 15

[Mark Banner (:standard8)]

https://hg.mozilla.org/comm-central/rev/9f6cfef5475d

Comment 16

[ISHIKAWA, Chiaki]

(In reply to Mark Banner (:standard8) from comment #13)
> Should be using kNewsSortOffset rather than 9000 here. r=me with this fixed
> - I'll update the patch and land it for you.

Thank you!

Comment 17

[ISHIKAWA, Chiaki]

I re-read the post:

https://bugzilla.mozilla.org/show_bug.cgi?id=761987#c173

It seems then whichever thread that is executing the hash table manipulation
code is doing this without acquiring the lock, I think. That is bad.

I thought it was only the cleanup code that was missing the lock.
But there are other places where the lock is missing obviously...

Comment 19

[Al Billings [:abillings]]

This has been determined to be non-qualifying for bounty because of the sec-moderate for rating.

If everyone agrees, can we open this bug?