Closed
Bug 806291
Opened 13 years ago
Closed 13 years ago
Use of uninitialised value of size 4 in js::Int32ToString
Categories
(MailNews Core :: Backend, defect)
MailNews Core
Backend
Tracking
(firefox-esr10 unaffected, firefox-esr17 unaffected, b2g18 unaffected)
RESOLVED
FIXED
Thunderbird 19.0
Tracking | Status | |
---|---|---|
firefox-esr10 | --- | unaffected |
firefox-esr17 | --- | unaffected |
b2g18 | --- | unaffected |
People
(Reporter: gkw, Assigned: ishikawa)
References
(Blocks 1 open bug)
Details
(Keywords: reporter-external, sec-moderate, valgrind)
Attachments
(2 files)
863 bytes,
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
2.45 MB,
text/plain
|
Details |
(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==
Assignee: general → wmccloskey
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.
Assignee: wmccloskey → general
![]() |
Reporter | |
Updated•13 years ago
|
Assignee: general → nobody
Component: JavaScript Engine → Backend
Product: Core → MailNews Core
Comment 2•13 years ago
|
||
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?
Assignee | ||
Comment 3•13 years ago
|
||
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.).
Assignee | ||
Comment 4•13 years ago
|
||
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•13 years ago
|
||
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.
Assignee | ||
Comment 6•13 years ago
|
||
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(?!)
Assignee | ||
Comment 7•13 years ago
|
||
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
Assignee | ||
Comment 8•13 years ago
|
||
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•13 years ago
|
||
(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•13 years ago
|
||
(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.
Assignee | ||
Comment 11•13 years ago
|
||
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!
![]() |
Reporter | |
Comment 12•13 years ago
|
||
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.
Attachment #677624 -
Flags: review?(mbanner)
Comment 13•13 years ago
|
||
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.
Attachment #677624 -
Flags: review?(mbanner) → review+
Comment 14•13 years ago
|
||
(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•13 years ago
|
||
Assignee: nobody → ishikawa
Status: NEW → RESOLVED
Closed: 13 years ago
OS: Linux → All
QA Contact: general
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 19.0
Assignee | ||
Comment 16•13 years ago
|
||
(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!
Updated•13 years ago
|
Keywords: sec-moderate
Updated•13 years ago
|
status-firefox-esr10:
--- → unaffected
status-firefox-esr17:
--- → unaffected
Assignee | ||
Comment 17•13 years ago
|
||
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...
![]() |
Reporter | |
Updated•13 years ago
|
Flags: sec-bounty?
Comment 19•12 years ago
|
||
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?
Flags: sec-bounty? → sec-bounty-
Updated•12 years ago
|
status-b2g18:
--- → unaffected
Updated•10 years ago
|
Group: core-security → core-security-release
Updated•9 years ago
|
Group: core-security-release
Updated•1 year ago
|
Keywords: reporter-external
You need to log in
before you can comment on or make changes to this bug.
Description
•