809060 - Uninitialized value usage in ./mailnews/news/src/nsNntpIncomingServer.cpp

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[ISHIKAWA, Chiaki]

Uninitialized value usage in ./mailnews/news/src/nsNntpIncomingServer.cpp

Found by valgrind run. See bug 803816 about mozmill run of TB.

Source version:
comm-central thunderbird.
$ hg identify
1016cef82fd8+ tip
ishikawa@debian-vm:~/TB-NEW/TB-3HG/new-src$ cd mozilla
ishikawa@debian-vm:~/TB-NEW/TB-3HG/new-src/mozilla$ hg identify
a517f7ea5bef+ tip

Valgrind log

==28703== Conditional jump or move depends on uninitialised value(s)
==28703==    at 0x5D4F30E: nsNntpIncomingServer::CloseCachedConnections() (nsNntpIncomingServer.cpp:437)
==28703==    by 0x5B31F1B: nsMsgIncomingServer::Shutdown() (nsMsgIncomingServer.cpp:207)
==28703==    by 0x5B5604D: hashShutdown(nsACString_internal const&, nsCOMPtr<nsIMsgIncomingServer>&, void*) (nsMsgAccountManager.cpp:1130)
==28703==    by 0x5B561B2: nsBaseHashtable<nsCStringHashKey, nsCOMPtr<nsIMsgIncomingServer>, nsIMsgIncomingServer*>::s_EnumStub(PLDHashTable*, PLDHashEntryHdr*, unsigned int, void*) (nsBaseHashtable.h:419)
==28703==    by 0x5FB697B: PL_DHashTableEnumerate (pldhash.cpp:716)
==28703==    by 0x5B5825B: nsMsgAccountManager::ShutdownServers() (nsBaseHashtable.h:223)
==28703==    by 0x5B5E557: nsMsgAccountManager::Shutdown() (nsMsgAccountManager.cpp:225)
==28703==    by 0x5B5E70D: nsMsgAccountManager::Observe(nsISupports*, char const*, unsigned short const*) (nsMsgAccountManager.cpp:297)
==28703==    by 0x5FC8D72: nsObserverList::NotifyObservers(nsISupports*, char const*, unsigned short const*) (nsObserverList.cpp:99)
==28703==    by 0x5FC92FD: nsObserverService::NotifyObservers(nsISupports*, char const*, unsigned short const*) (nsObserverService.cpp:149)
==28703==    by 0x4C48A22: nsXREDirProvider::DoShutdown() (nsXREDirProvider.cpp:861)
==28703==    by 0x4C41917: ScopedXPCOMStartup::~ScopedXPCOMStartup() (nsAppRunner.cpp:1112)
==28703==    by 0x4C47166: XREMain::XRE_main(int, char**, nsXREAppData const*) (nsAppRunner.cpp:3887)
==28703==    by 0x4C473A3: XRE_main (nsAppRunner.cpp:3941)
==28703==    by 0x41ADE45: (below main) (libc-start.c:228)
==28703==  Uninitialised value was created by a heap allocation
==28703==    at 0x40271C4: malloc (vg_replace_malloc.c:270)
==28703==    by 0x4041E10: moz_xmalloc (mozalloc.cpp:54)
==28703==    by 0x6: ???
==28703==    by 0x357264: ???
==28703==


Observation:

grep -n mGetOnlyNew *.cpp *.h

nsNntpIncomingServer.cpp:437:  if (!mGetOnlyNew && !mHostInfoLoaded)
nsNntpIncomingServer.cpp:946:  mGetOnlyNew = aGetOnlyNew;
nsNntpIncomingServer.cpp:1074:    if (mGetOnlyNew && (mGroupsOnServer.IndexOf(path) == mGroupsOnServer.NoIndex))
nsNntpIncomingServer.cpp:1131:  if (!mGetOnlyNew && !mHostInfoLoaded)
nsNntpIncomingServer.h:115:    bool     mGetOnlyNew;

grep -n mHostInfoLoaded *.cpp *.h

nsNntpIncomingServer.cpp:96:  mHostInfoLoaded = false;
nsNntpIncomingServer.cpp:437:  if (!mGetOnlyNew && !mHostInfoLoaded)
nsNntpIncomingServer.cpp:853:  mHostInfoLoaded = false;
nsNntpIncomingServer.cpp:943:  mHostInfoLoaded = false;
nsNntpIncomingServer.cpp:953:  // mHostInfoLoaded can be false if we failed to load anything
nsNntpIncomingServer.cpp:954:  if (aForceToServer || !mHostInfoLoaded || (mVersion != VALID_VERSION)) {
nsNntpIncomingServer.cpp:1131:  if (!mGetOnlyNew && !mHostInfoLoaded)
nsNntpIncomingServer.cpp:1234:      mHostInfoLoaded = true;
nsNntpIncomingServer.h:121:    bool mHostInfoLoaded;

Since mHostInfoLoaded is set to false on initialization of the
class object (see below)

>nsNntpIncomingServer::nsNntpIncomingServer()
>{
>  mNewsrcHasChanged = false;
>
>  mHostInfoLoaded = false;
>  mHostInfoHasChanged = false;
>  mVersion = INVALID_VERSION;


there seems to be an execution path in which mGetOnlyNew 
is not set, and is referenced later.

Full log of make mozmill TB run under valgrind is in bug 803816

(If this is a valid security bug, maybe it should be set as such.)

Comment 1

[Andrew McCreight [:mccr8]]

This sounds sketchy, so I'm setting it to high. Feel free to adjust up or down as needed.
https://wiki.mozilla.org/Security_Severity_Ratings

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Mark, what would be the best way forward for this?

Joshua, you've touch NNTP before, would this be interesting?

Comment 3

[Daniel Veditz [:dveditz]]

Assigning to Mark to find a real assignee

Comment 4

[ISHIKAWA, Chiaki]

Attachment: A patch with some extra stuff for valgrind/memcheck

Hi,

I have learned to use some microscopic probe of valgrind/memcheck, and indeed
found that the uninitialized problem is caused by the uninitialized value of
mGetOnlyNew caused the memcheck warning.

I am attaching the patch for initializing mGetOnlyNew to true.
I chose the value of true since 
it has contained garbage from heap, and i.e., most likely non-zero, I decided to
set it true. Patched thunderbird completed "make mozmill" successfully I no longer see the warning from this particular issue.

I can change the value to false if necessary, but anyway the cause and
the fix seems to be finally determined.

TIA

Comment 5

[Julian Seward [:jseward]]

All you need from that patch (IMO) is the fix

mGetOnlyNew = true;

The rest of it is debugging code and I don't think it can land as-is;
certainly not with #include <valgrind/memcheck.h> unprotected as that
will break the build on other targets.

Comment 6

[ISHIKAWA, Chiaki]

Attachment: Simplified patch

I simplified the patch to contain the assignment to |mGetOnlyNew| only.
The previous patch was to show how VALGRIND macros were used to hunt down the 
uninitialized values. (For other people who might be interested in hunting down such bugs.)

TIA

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 720392 [details] [diff] [review]
A patch with some extra stuff for valgrind/memcheck

I'm guessing this patch is now obsolete.

Comment 8

[Mark Banner (:standard8)]

The more I look at this bug, the more I think it isn't a security issue, certainly not sec-high. There are three locations where we currently check the value for true or false:

http://mxr.mozilla.org/comm-central/search?string=mGetOnlyNew

none of those are critical or would likely to cause a crash if the value was uninitialized - especially as it is being treated as a boolean.

Dan, can you take a look and confirm please?

Comment 9

[Joshua Cranmer [:jcranmer]]

(In reply to Mark Banner (:standard8) from comment #8)
> The more I look at this bug, the more I think it isn't a security issue,
> certainly not sec-high. There are three locations where we currently check
> the value for true or false:
> 
> http://mxr.mozilla.org/comm-central/search?string=mGetOnlyNew
> 
> none of those are critical or would likely to cause a crash if the value was
> uninitialized - especially as it is being treated as a boolean.

So, mGetNew is initialized when startPopulating is called. Of the three other places, two are in callbacks that will only be called in a successor call to startPopulating (stopPopulating and setAsSubscribed), and the third is called in closeCachedConnections, where it is jointly dependent with mHostInfoLoaded. The only consequence of calling the gated method is that we will empty out a nonessential file which is a cache of our seen newsgroups during subscribed, so the security sensitivity of this is extremely low.

Comment 10

[ISHIKAWA, Chiaki]

(In reply to Mark Banner (:standard8) from comment #8)
> The more I look at this bug, the more I think it isn't a security issue,
> certainly not sec-high. 

I think so, too. It looks that the uninitialized value causes possibly the too early purge of cache entry/clear, but the next use will bring it in any way.
At least, with the patch, we now have a deterministic behavior (from the viewpoint of the user) instead of relying on the random junk on the heap.

TIA

Comment 11

[ISHIKAWA, Chiaki]

Sorry, this was a comment intended for another bug 845187.
There the uninitialized variable is used to check whether to purge/clear old
cache entry.
Sorry for the confusion.


(In reply to ISHIKAWA, chiaki from comment #10)
> (In reply to Mark Banner (:standard8) from comment #8)
> > The more I look at this bug, the more I think it isn't a security issue,
> > certainly not sec-high. 
> 
> I think so, too. It looks that the uninitialized value causes possibly the
> too early purge of cache entry/clear, but the next use will bring it in any
> way.
> At least, with the patch, we now have a deterministic behavior (from the
> viewpoint of the user) instead of relying on the random junk on the heap.
> 
> TIA

Comment 12

[Mark Banner (:standard8)]

Per discussion in comment 8 and comment 9 downgrading to sec-low.

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 720744 [details] [diff] [review]
Simplified patch

Thanks for the patch and the investigation.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/2275efa4b6d3