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

RESOLVED FIXED in Thunderbird 47.0

Status

defect
--
major
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ishikawa, Assigned: ishikawa)

Tracking

unspecified
Thunderbird 47.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Assignee: nobody → ishikawa
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
Attachment #8713354 - Flags: review?(mkmelin+mozilla)
Perhaps some good will come of it :)
Component: Untriaged → Database
Product: Thunderbird → MailNews Core
See Also: → 1203977
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.
(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 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 - "
Attachment #8713354 - Flags: review?(mkmelin+mozilla) → review+
(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.
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
Attachment #8713354 - Attachment is obsolete: true
Attachment #8714613 - Flags: review+
Attachment #8714613 - Attachment description: uninitialized-memory-access.patch → uninitialized-memory-access.patch carrying over r=mkmelin+mozilla
Keywords: checkin-needed
Blocks: 1246048
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
Status: NEW → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 47.0
You need to log in before you can comment on or make changes to this bug.