Closed Bug 344157 Opened 14 years ago Closed 11 months ago
Possible crashes via Open
MDB and Get MDBFactory
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.
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?
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
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 email@example.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.