Closed Bug 344157 Opened 14 years ago Closed 11 months ago

Possible crashes via OpenMDB and GetMDBFactory

Categories

(MailNews Core :: Backend, defect, critical)

defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 65.0

People

(Reporter: standard8, Assigned: aceman)

Details

(Keywords: crash, testcase-wanted)

Attachments

(1 file)

There are several functions called OpenMDB and GetMDBFactory in the mailnews backend.

GetMDBFactory calls nsIMdbFactoryFactory::GetMdbFactory which will return NS_OK and a valid object or NS_ERROR_OUT_OF_MEMORY and a null pointer.

The GetMDBFactory returns the pointer and drops the error result. Whilst OpenMDB checks this for null (out of memory case I far as I can tell), it returns NS_OK to the caller with a null pointer, hence the caller doesn't realise that the get has failed.

I think this may lead to confusion and possible crashes, though I haven't got a definite case yet.
let's assume crashes
Severity: normal → critical
Keywords: crash, helpwanted
Priority: -- → P2
Product: Core → MailNews Core
Keywords: testcase-wanted
Assignee: bugzilla → nobody
Priority: P2 → --
Is this still valid?
Using this search:
http://mxr.mozilla.org/comm-central/ident?i=GetMDBFactory
there is only 
nsMsgFolderCache::GetMDBFactory
nsAddrDatabase::GetMDBFactory
nsMsgDatabase::GetMDBFactory

All of them return void and the pointer. There is no nsIMdbFactoryFactory.
You want to look at the OpenMDB functions - although GetMDBFactory returns void, its OpenMDB at fault.
I can see OpenMDB return NS_OK even when it got NULL from GetMDBFactory. But I can't see it returning any null pointer. So should I just change the return value to NS_ERROR_OUT_OF_MEMORY in this case?
(In reply to :aceman from comment #4)
> I can see OpenMDB return NS_OK even when it got NULL from GetMDBFactory. But
> I can't see it returning any null pointer. So should I just change the
> return value to NS_ERROR_OUT_OF_MEMORY in this case?

I know there's no response to your question, but any knowledge gained in the 7 years that helps you proceed?
If not, who might we ask?
Flags: needinfo?(acelists)
Keywords: helpwanted
Attached patch 344157.patchSplinter Review
Yes I think I see the problem now.
I have made GetMDBFactory() return the result and made OpenMDB() propagate it up properly.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a75cacd4c62f3bb6cd713048ecfefa2eb16b404a
Assignee: nobody → acelists
Flags: needinfo?(acelists)
Attachment #9029036 - Flags: review?(jorgk)
Status: NEW → ASSIGNED
Target Milestone: --- → Thunderbird 65.0
Comment on attachment 9029036 [details] [diff] [review]
344157.patch

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

Fine, thanks.

::: mailnews/base/src/nsMsgFolderCache.cpp
@@ +140,1 @@
>      ret = mdbFactory->MakeEnv(nullptr, &m_mdbEnv);

This needs to be re-indented.

::: mailnews/db/msgdb/src/nsMsgDatabase.cpp
@@ +1322,1 @@
>      ret = mdbFactory->MakeEnv(NULL, &m_mdbEnv);

Re-indent.

@@ -1417,5 @@
>          }
>        }
>      }
> -  }
> -#ifdef DEBUG_David_Bienvenu

Don't remove this, it gives the code such a personal touch. Hint: Joke!
Attachment #9029036 - Flags: review?(jorgk) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/80212fc6395f
properly check success of GetMDBFactory(). r=jorgk
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.