1246048 - Beware that GetSummaryValid(&valid) does not always set a value to valid.

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Description

[ISHIKAWA, Chiaki]

I reported a bug in 1243895 that was detected by valgrind and a patch.
This bugzilla addresses two more instances where GetSummaryValid() is called, but the argument may not be set, thus may cause uninitialized memory access during run-time.
+++ This bug was initially created as a clone of Bug #1243895 +++

TB. Uninitialized memory access - nsMsgDatabase.cpp. Failure to check the return value of GetSummaryValid(&valid);


Running C-C TB under valgrind after creating a few extra IMAP accounts to look into 
Bug 1203977 - Thunderbird crashes in nsImapMailFolder::ParseMsgHdrs
I hit upon an uninitialized value access (I have not seen this for a couple of years now, but
obviously my valgrind coverage is not exhaustive.)

The valgrind report is as follows.

E.g.: The line number 1255 of nsMsgDatabase.cpp is probably off from the
pristine tree since I have many local patches.

--DOMWINDOW == 26 (0x34821cf0) [pid = 14734] [serial = 28] [outer = (nil)] [url = chrome://messenger/content/msgAccountCentral.xul]
==14734== Thread 1:
==14734== Conditional jump or move depends on uninitialised value(s)
==14734==    at 0xF4D84E2: nsMsgDatabase::CheckForErrors(nsresult, bool, nsMsgDBService*, nsIFile*) (nsMsgDatabase.cpp:1255)
==14734==    by 0xF4D8739: nsMsgDatabase::OpenInternal(nsMsgDBService*, nsIFile*, bool, bool, bool) (nsMsgDatabase.cpp:1221)
==14734==    by 0xF4D8D4D: nsMsgDatabase::Open(nsMsgDBService*, nsIFile*, bool, bool) (nsMsgDatabase.cpp:1188)
==14734==    by 0xF4CE8A0: nsMailDatabase::Open(nsMsgDBService*, nsIFile*, bool, bool) (nsMailDatabase.cpp:52)
==14734==    by 0xF4D7145: nsMsgDBService::OpenMailDBFromFile(nsIFile*, nsIMsgFolder*, bool, bool, nsIMsgDatabase**) (nsMsgDatabase.cpp:356)
==14734==    by 0xF3BE13A: nsMsgDBFolder::OpenBackupMsgDatabase() (nsMsgDBFolder.cpp:317)
==14734==    by 0xF56B699: nsImapMailFolder::UpdateImapMailboxInfo(nsIImapProtocol*, nsIMailboxSpec*) (nsImapMailFolder.cpp:2748)
==14734==    by 0xF5D7A14: (anonymous namespace)::SyncRunnable2<nsIImapMailFolderSink, nsIImapProtocol*, nsIMailboxSpec*>::Run() (nsSyncRunnableHelpers.cpp:146)
==14734==    by 0xF7EB33F: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:997)
==14734==    by 0xF82FCD8: NS_ProcessNextEvent(nsIThread*, bool) (nsThreadUtils.cpp:297)
==14734==    by 0x12CC2495: nsXULWindow::ShowModal() (nsXULWindow.cpp:400)
==14734==    by 0x12CBB3C1: nsContentTreeOwner::ShowAsModal() (nsContentTreeOwner.cpp:577)
==14734== 

mailnews/db/msgdb/src/nsMsgDatabase.cpp with my local line numbers.

1255 of nsMsgDatabase.cpp
 1250	    {
 1251	      if (!newFile && summaryFileExists)
 1252	      {
 1253	        bool valid;
 1254	        GetSummaryValid(&valid);
 1255	        if (!valid)
 1256	          err = NS_MSG_ERROR_FOLDER_SUMMARY_OUT_OF_DATE;
 1257	      }
 1258	      // compare current version of db versus filed out version info.
 1259	      uint32_t version;

So this only means that GetSummaryValid() is returning an error without touching
passed argument.

Checking GetSummaryValid() using mxr.mozilla.org, I find a few implementations,
and one of them certainly looks returning an error without setting the argument.

Ugh...

The characteristics of C-C TB code not to check the return value of lower-level code. Sigh.

Patch attached.

PS: I have no idea if I have hit into a bug of many IMAP account or something. I cannot even send an e-mail after the creation of a few extra imap accounts. /tmp/nsmail.tmp cannot be created !?
Stay tuned.

Comment 1

[ISHIKAWA, Chiaki]

Attachment: GetSummaryValid-returns-without-setting-valid.patch

Here is the patch. I have decided not to check the return value but simply set the variable passed GetSummaryValid() per https://bugzilla.mozilla.org/show_bug.cgi?id=1243895#c4
(bug 1243895 comment 4)

TIA

Comment 2

[:aceman]

Comment on attachment 8716138 [details] [diff] [review]
GetSummaryValid-returns-without-setting-valid.patch

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

This seems fine as a robustness measure, but shouldn't we actually fix the function? Or actualy check the return value in addition to the 'valid' return argument? The function can return via  NS_ENSURE_ARG_POINTER() without setting the argument as you noticed. Checking the nsresult return value would catch these cases.

Comment 3

[ISHIKAWA, Chiaki]

(In reply to :aceman from comment #2)
> Comment on attachment 8716138 [details] [diff] [review]
> GetSummaryValid-returns-without-setting-valid.patch
> 
> Review of attachment 8716138 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems fine as a robustness measure, but shouldn't we actually fix the
> function? Or actualy check the return value in addition to the 'valid'
> return argument? The function can return via  NS_ENSURE_ARG_POINTER()
> without setting the argument as you noticed. Checking the nsresult return
> value would catch these cases.

Let me see.

There are three implementations of GetSummary Valid() in C-C tree:

(1) This one sets a value to the argument always. 
http://mxr.mozilla.org/comm-central/source/mailnews/db/msgdb/src/nsImapMailDatabase.cpp#25

25 NS_IMETHODIMP	nsImapMailDatabase::GetSummaryValid(bool *aResult)
26 {
27   NS_ENSURE_ARG_POINTER(aResult);
28   if (m_dbFolderInfo)
29   {
30     uint32_t version;
31     m_dbFolderInfo->GetVersion(&version);
32     *aResult = (GetCurVersion() == version);
33   }
34   else
35       *aResult = false;
36 
37   return NS_OK;
38 }

(2) This one definitely does not set a value to the argument on its
early return cases. 

I am not even sure if the last call
msgStore->IsSummaryFileValid(m_folder, this, aResult)
sets a value to *aResult when IsSummaryFileValid() returns an error.

http://mxr.mozilla.org/comm-central/source/mailnews/db/msgdb/src/nsMailDatabase.cpp#102

This misses !aResult check. Should be there. NEW BUG.

NS_IMETHODIMP nsMailDatabase::GetSummaryValid(bool *aResult)
103 {
104   uint32_t version;
105   m_dbFolderInfo->GetVersion(&version);
106   if (GetCurVersion() != version)
107   {
108     *aResult = false;
109     return NS_OK;
110   }
111   nsCOMPtr<nsIMsgPluggableStore> msgStore;
112   if (!m_folder)
113     return NS_ERROR_NULL_POINTER;
114   nsresult rv = m_folder->GetMsgStore(getter_AddRefs(msgStore));
115   NS_ENSURE_SUCCESS(rv, rv);
116   return msgStore->IsSummaryFileValid(m_folder, this, aResult);
117 }

(3) This one is easy.
This always return a value as long as aResult is not nullptr.
http://mxr.mozilla.org/comm-central/source/mailnews/db/msgdb/src/nsMsgDatabase.cpp#4050
4050 NS_IMETHODIMP  nsMsgDatabase::GetSummaryValid(bool *aResult)
4051 {
4052   NS_ENSURE_ARG_POINTER(aResult);
4053   *aResult = true;
4054   return NS_OK;
4055 }

Well, I can certainly check the return values.
Did you read the comment of rkent in the quoted place?
> I have decided not to check the return value but simply set the variable passed > GetSummaryValid() per https://bugzilla.mozilla.org/show_bug.cgi?id=1243895#c4
> (bug 1243895 comment 4)

TIA

PS: I am more disturbed now how IsSummaryFileValid() are written. There seem to be
a few checks missing in there, too (!)

Comment 4

[:aceman]

(In reply to ISHIKAWA, Chiaki from comment #3)
> There are three implementations of GetSummary Valid() in C-C tree:
> 
> (1) This one sets a value to the argument always. 
> http://mxr.mozilla.org/comm-central/source/mailnews/db/msgdb/src/
> nsImapMailDatabase.cpp#25
> 
> 25 NS_IMETHODIMP	nsImapMailDatabase::GetSummaryValid(bool *aResult)
> 26 {
> 27   NS_ENSURE_ARG_POINTER(aResult);
> 28   if (m_dbFolderInfo)
> 29   {
> 30     uint32_t version;
> 31     m_dbFolderInfo->GetVersion(&version);
> 32     *aResult = (GetCurVersion() == version);
> 33   }
> 34   else
> 35       *aResult = false;
> 36 
> 37   return NS_OK;
> 38 }
> 
> (2) This one definitely does not set a value to the argument on its
> early return cases. 

Yes, on the other hand in this case if it returns on NS_ENSURE_ARG_POINTER() then it can't set the argument as it is nullptr ;)

> I am not even sure if the last call
> msgStore->IsSummaryFileValid(m_folder, this, aResult)
> sets a value to *aResult when IsSummaryFileValid() returns an error.

For safety I would assume it does not.

> http://mxr.mozilla.org/comm-central/source/mailnews/db/msgdb/src/
> nsMailDatabase.cpp#102
> 
> This misses !aResult check. Should be there. NEW BUG.

Yes, please do.

> NS_IMETHODIMP nsMailDatabase::GetSummaryValid(bool *aResult)
> 103 {
> 104   uint32_t version;
> 105   m_dbFolderInfo->GetVersion(&version);
> 106   if (GetCurVersion() != version)
> 107   {
> 108     *aResult = false;
> 109     return NS_OK;
> 110   }
> 111   nsCOMPtr<nsIMsgPluggableStore> msgStore;
> 112   if (!m_folder)
> 113     return NS_ERROR_NULL_POINTER;
> 114   nsresult rv = m_folder->GetMsgStore(getter_AddRefs(msgStore));
> 115   NS_ENSURE_SUCCESS(rv, rv);
> 116   return msgStore->IsSummaryFileValid(m_folder, this, aResult);
> 117 }

I probably meant the !m_folder case, where aResult is untouched.

> 111   NS_ENSURE_STATE(m_folder);
> 112   nsCOMPtr<nsIMsgPluggableStore> msgStore;

> Well, I can certainly check the return values.
> Did you read the comment of rkent in the quoted place?
> > I have decided not to check the return value but simply set the variable passed > GetSummaryValid() per https://bugzilla.mozilla.org/show_bug.cgi?id=1243895#c4
> > (bug 1243895 comment 4)

Thanks for pointing this out. But I think that comment only applies in cases where we then do the same whether we got an error or the summary is invalid.

There may be cases where we want to do different action (e.g. 1) report the error in case of NS_FAILED(rv) and 2) delete .msf file if it is invalid (but not if we don't know))

But yes, it seems the cases you patch are of the first type so using just the 'valid' variable seems fine. It just hides if there is an error and we do not report it now, it may blow up later in the code.
In the case where we intentionally do not check the return value we use the convention of prepending (void) '(void) db->GetSummaryValid(&valid)' so that it is clear and prevents the itching of somebody else to add the checking :)

Comment 5

[:aceman]

Comment on attachment 8716138 [details] [diff] [review]
GetSummaryValid-returns-without-setting-valid.patch

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

::: mailnews/local/src/nsLocalMailFolder.cpp
@@ +2051,5 @@
>      // this will kick off a reparse if the db is out of date.
>      rv = localInbox->GetDatabaseWithReparse(nullptr, aWindow, getter_AddRefs(db));
>      if (NS_SUCCEEDED(rv))
>      {
> +      valid = false; // GetSummaryValid may return without setting valid

Does this actually do anything? Isn't 'valid' set to false above at line 2049 (doesn't seem to be touched since that line)?

@@ +3483,5 @@
>      if (mFlags & nsMsgFolderFlags::Inbox)
>      {
>        if (mDatabase && mCheckForNewMessagesAfterParsing)
>        {
> +        bool valid = false; // GetSummaryValid may return without setting valid.

Isn't false the default value that the compiler initializes all bool variables? But yes, maybe it is undefined by C++ and may not be guaranteed with all compilers.

Comment 6

[ISHIKAWA, Chiaki]

(In reply to :aceman from comment #5)
> Comment on attachment 8716138 [details] [diff] [review]
> GetSummaryValid-returns-without-setting-valid.patch
> 
> Review of attachment 8716138 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mailnews/local/src/nsLocalMailFolder.cpp
> @@ +2051,5 @@
> >      // this will kick off a reparse if the db is out of date.
> >      rv = localInbox->GetDatabaseWithReparse(nullptr, aWindow, getter_AddRefs(db));
> >      if (NS_SUCCEEDED(rv))
> >      {
> > +      valid = false; // GetSummaryValid may return without setting valid
> 
> Does this actually do anything? Isn't 'valid' set to false above at line
> 2049 (doesn't seem to be touched since that line)?

Aha, you are right. But then, due to the distance, I obviously missed it.
I would like the initialization to be near where the use is.
Let me think about it.

> 
> @@ +3483,5 @@
> >      if (mFlags & nsMsgFolderFlags::Inbox)
> >      {
> >        if (mDatabase && mCheckForNewMessagesAfterParsing)
> >        {
> > +        bool valid = false; // GetSummaryValid may return without setting valid.
> 
> Isn't false the default value that the compiler initializes all bool
> variables? But yes, maybe it is undefined by C++ and may not be guaranteed
> with all compilers.

No, I don't think so. If it were the case, then the original bug 1243895 
would not have been reported by valgrind.


===

(In reply to :aceman from comment #4)
> (In reply to ISHIKAWA, Chiaki from comment #3)
> > There are three implementations of GetSummary Valid() in C-C tree:
> > 
> > (1) This one sets a value to the argument always. 
> > http://mxr.mozilla.org/comm-central/source/mailnews/db/msgdb/src/
> > nsImapMailDatabase.cpp#25
> > 
> > 25 NS_IMETHODIMP	nsImapMailDatabase::GetSummaryValid(bool *aResult)
> > 26 {
> > 27   NS_ENSURE_ARG_POINTER(aResult);
> > 28   if (m_dbFolderInfo)
> > 29   {
> > 30     uint32_t version;
> > 31     m_dbFolderInfo->GetVersion(&version);
> > 32     *aResult = (GetCurVersion() == version);
> > 33   }
> > 34   else
> > 35       *aResult = false;
> > 36 
> > 37   return NS_OK;
> > 38 }
> > 
> > (2) This one definitely does not set a value to the argument on its
> > early return cases. 
> 
> Yes, on the other hand in this case if it returns on NS_ENSURE_ARG_POINTER()
> then it can't set the argument as it is nullptr ;)
> 
> > I am not even sure if the last call
> > msgStore->IsSummaryFileValid(m_folder, this, aResult)
> > sets a value to *aResult when IsSummaryFileValid() returns an error.
> 
> For safety I would assume it does not.

I agree.

> 
> > http://mxr.mozilla.org/comm-central/source/mailnews/db/msgdb/src/
> > nsMailDatabase.cpp#102
> > 
> > This misses !aResult check. Should be there. NEW BUG.
> 
> Yes, please do.
> 

Another bug then.

> > NS_IMETHODIMP nsMailDatabase::GetSummaryValid(bool *aResult)
> > 103 {
> > 104   uint32_t version;
> > 105   m_dbFolderInfo->GetVersion(&version);
> > 106   if (GetCurVersion() != version)
> > 107   {
> > 108     *aResult = false;
> > 109     return NS_OK;
> > 110   }
> > 111   nsCOMPtr<nsIMsgPluggableStore> msgStore;
> > 112   if (!m_folder)
> > 113     return NS_ERROR_NULL_POINTER;
> > 114   nsresult rv = m_folder->GetMsgStore(getter_AddRefs(msgStore));
> > 115   NS_ENSURE_SUCCESS(rv, rv);
> > 116   return msgStore->IsSummaryFileValid(m_folder, this, aResult);
> > 117 }
> 
> I probably meant the !m_folder case, where aResult is untouched.
> 
> > 111   NS_ENSURE_STATE(m_folder);
> > 112   nsCOMPtr<nsIMsgPluggableStore> msgStore;
> 
> > Well, I can certainly check the return values.
> > Did you read the comment of rkent in the quoted place?
> > > I have decided not to check the return value but simply set the variable passed > GetSummaryValid() per https://bugzilla.mozilla.org/show_bug.cgi?id=1243895#c4
> > > (bug 1243895 comment 4)
> 
> Thanks for pointing this out. But I think that comment only applies in cases
> where we then do the same whether we got an error or the summary is invalid.
> 
> There may be cases where we want to do different action (e.g. 1) report the
> error in case of NS_FAILED(rv) and 2) delete .msf file if it is invalid (but
> not if we don't know))
> 
> But yes, it seems the cases you patch are of the first type so using just
> the 'valid' variable seems fine. It just hides if there is an error and we
> do not report it now, it may blow up later in the code.
> In the case where we intentionally do not check the return value we use the
> convention of prepending (void) '(void) db->GetSummaryValid(&valid)' so that
> it is clear and prevents the itching of somebody else to add the checking :)

Placing "(void)" would be a good idea.
Currently, there are so many lo-level function calls that exist but without
being checked of their return values.
It is probably OK to disregard the return values of some such calls, but
it is NOT OBVIOUS to people who want to maintain the code whether ignoring the value
is OK or not.

Maybe a student summer project can do such a rewrite.

Comment 7

[Jorg K (CEST = GMT+2)]

Comment on attachment 8716138 [details] [diff] [review]
GetSummaryValid-returns-without-setting-valid.patch

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

Stealing another review. The first line you added, we don't need. The second one: Yes, please. r+ with this fixed.

::: mailnews/local/src/nsLocalMailFolder.cpp
@@ +2051,5 @@
>      // this will kick off a reparse if the db is out of date.
>      rv = localInbox->GetDatabaseWithReparse(nullptr, aWindow, getter_AddRefs(db));
>      if (NS_SUCCEEDED(rv))
>      {
> +      valid = false; // GetSummaryValid may return without setting valid

Please remove this. As Aceman said, this is set to false a few lines above, so we don't need this.

@@ +3483,5 @@
>      if (mFlags & nsMsgFolderFlags::Inbox)
>      {
>        if (mDatabase && mCheckForNewMessagesAfterParsing)
>        {
> +        bool valid = false; // GetSummaryValid may return without setting valid.

Good practise to initialise "out" variables. The compiler won't initialise local variables. So, yes, please.

Comment 8

[ISHIKAWA, Chiaki]

Thank you for the review. I will create a patch over the weekend along the line of the suggestions.

Comment 9

[ISHIKAWA, Chiaki]

Attachment: GetSummaryValid-returns-without-setting-valid.patch carrying over [e-mail]

I updated the description and added r=[e-mail]

Comment 10

[ISHIKAWA, Chiaki]

I will put the checkin-needed keyword, tomorrow if there is no objection.
TIA

Comment 11

[Jorg K (CEST = GMT+2)]

OK, I can check it in, it needs to read: r=jorgk.
Don't worry, no new patch required, we'll fix it upon check-in.

Comment 12

[:aceman]

https://hg.mozilla.org/comm-central/rev/d1588f44c003

Comment 13

[Jorg K (CEST = GMT+2)]

Comment on attachment 8721684 [details] [diff] [review]
GetSummaryValid-returns-without-setting-valid.patch　carrying over [e-mail]

[Approval Request Comment]
Regression caused by (bug #): No regression.
User impact if declined: Who knows.

I've dug through the code and there are paths (error cases) where the output parameter is not populated. So for general safety I approve/request uplift.

Also, I think we should appreciate the work of Chiaki who works in the underwood to make TB better by weeding out possible sources of error like this one.

Comment 14

[ISHIKAWA, Chiaki]

(In reply to Jorg K (GMT+1) from comment #11)
> OK, I can check it in, it needs to read: r=jorgk.
> Don't worry, no new patch required, we'll fix it upon check-in.

Thank you for fixing the approval signature. Also, thank　　you for the nice words in comment 13: witout valgrind/memcheck I could not have realized the issue. Strange, though, the error cropped up only now. I have been running valgrind/memcheck quite sometime, and so it must be that the code change elsewhere has caused the issue surface lately.

I vaguely recall that there was an obscure issue caused by GetSummaryValid() returning strange value (maybe not returning it) after compaction. I will dig into it again.


TIA

Comment 15

[Jorg K (CEST = GMT+2)]

Aurora (TB 46):
https://hg.mozilla.org/releases/comm-aurora/rev/e41c21e476da

Comment 16

[Kent James (:rkent)]

Comment on attachment 8721684 [details] [diff] [review]
GetSummaryValid-returns-without-setting-valid.patch　carrying over [e-mail]

http://hg.mozilla.org/releases/comm-esr45/rev/c82da2901ce8