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

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Description

[ISHIKAWA, Chiaki]

Attachment: uninitialized-memory-access.patch

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]

Comment on attachment 8713354 [details] [diff] [review]
uninitialized-memory-access.patch

I am putting mkmelin+mozilla as a possible reviewer since he is one of the "suggested reviewers" that is automatically shown in the reviewer selection process. Please feel free to re-assign.

TIA

Comment 2

[Wayne Mery (:wsmwk)]

Perhaps some good will come of it :)

Comment 3

[ISHIKAWA, Chiaki]

I notice there are a few other places where the return value of GetSummaryValid() is not checked. I will file another bug over the weekend.

Comment 4

[Kent James (:rkent)]

(In reply to ISHIKAWA, Chiaki from comment #3)
> I notice there are a few other places where the return value of
> GetSummaryValid() is not checked. I will file another bug over the weekend.

I don't think it is particularly useful to add error checks in cases where you would not change actions based on that. It creates extra work to review and checkin code, plus clutters the code with lots declarations and checks.

In the example that you have:

+        bool valid = false;
+        nsresult rv = GetSummaryValid(&valid);
+        if (NS_FAILED(rv) || !valid)
           err = NS_MSG_ERROR_FOLDER_SUMMARY_OUT_OF_DATE;

setting the default value "valid = false" was a legitimate change, as this particular code did not have any handling of a failure of GetSummaryValid. But the "rv" does not add any value, as you should generally assume (unless there is a known reason not to) that simple getters will not change their return value unless they are changing it to a valid value. So checking the rv really does nothing here. Now I would not change it in your patch, as what you did works and is fine, but please do not do a bunch of changes were you start adding nothing but checks of return value in code where the return value error does not actually change any behavior.

Not everyone will agree with me on this, but keep in mind that code churn has its cost.

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 8713354 [details] [diff] [review]
uninitialized-memory-access.patch

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

LTGM, but please start the hg commit message with "Bug 1243895 - "

Comment 6

[ISHIKAWA, Chiaki]

(In reply to Kent James (:rkent) from comment #4)
> (In reply to ISHIKAWA, Chiaki from comment #3)
> > I notice there are a few other places where the return value of
> > GetSummaryValid() is not checked. I will file another bug over the weekend.
> 
> I don't think it is particularly useful to add error checks in cases where
> you would not change actions based on that. It creates extra work to review
> and checkin code, plus clutters the code with lots declarations and checks.
> 
> In the example that you have:
> 
> +        bool valid = false;
> +        nsresult rv = GetSummaryValid(&valid);
> +        if (NS_FAILED(rv) || !valid)
>            err = NS_MSG_ERROR_FOLDER_SUMMARY_OUT_OF_DATE;
> 
> setting the default value "valid = false" was a legitimate change, as this
> particular code did not have any handling of a failure of GetSummaryValid.
> But the "rv" does not add any value, 

Thank you for the comment. It took me a few times to read your comment to get the message across.
The following part is a little difficult for me to understand.

> as you should generally assume (unless
> there is a known reason not to) that simple getters will not change their
> return value unless they are changing it to a valid value. 

I think for someone who is not familiar with the innards of C-C TB code (or M-C code) to figure out what are "SIMPLE" getters and what are not.
(I will file a different bugzilla for a bug in M-C portion of the tree. valgrind run
found it. The issue is subtle, but in that case, I think the return value ought to be checked, I suppose.)

> So checking the
> rv really does nothing here. Now I would not change it in your patch, as
> what you did works and is fine, but please do not do a bunch of changes were
> you start adding nothing but checks of return value in code where the return
> value error does not actually change any behavior.
> 
> Not everyone will agree with me on this, but keep in mind that code churn
> has its cost.

I will keep this in mind. Thank you.
(The patch series for enabling buffered write with necessary I/O error checks
added mechanical checks of I/O routines in places, but they are not "SIMPLE" getters and
have side-effects with bad results when they fail. So I think the checks are necessary.)


(In reply to Magnus Melin from comment #5)
> Comment on attachment 8713354 [details] [diff] [review]
> uninitialized-memory-access.patch
> 
> Review of attachment 8713354 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LTGM, but please start the hg commit message with "Bug 1243895 - "

Thank you, I wil add the "Bug 1243895 - " prefix and r=mkmelin+mozilla and upload again.

Comment 7

[ISHIKAWA, Chiaki]

Attachment: uninitialized-memory-access.patch carrying over r=mkmelin+mozilla

Here is the updated patch with new message.
I will put the checkin-needed keyword.

>I think for someone who is not familiar with the innards of C-C TB code (or M-C 
> code) to figure out what are "SIMPLE" getters and what are not.
"is difficult" at the end was missing in the typed sentence above.

TIA

Comment 8

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/9fa59c97f44dead8c62d2baacf1fc6533504ab5a
Bug 1243895 - Uninitialized memory access - nsMsgDatabase.cpp. Failure to check the return value of GetSummaryValid(&valid). r=mkmelin