Closed
Bug 425465
Opened 17 years ago
Closed 10 years ago
crash possible because fSearchResults = nsImapSearchResultSequence::CreateSearchResultSequence() isn't null checked
Categories
(MailNews Core :: Networking: IMAP, defect)
MailNews Core
Networking: IMAP
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: mkmelin, Unassigned)
References
()
Details
(Keywords: crash)
timeless in bug 341929 comment 3
problems i see:
1.
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/mailnews/imap/src/nsImapServerResponseParser.cpp&rev=1.139&mark=81,321,401,821
81 bienvenu 1.91 fSearchResults =
nsImapSearchResultSequence::CreateSearchResultSequence();
isn't null checked, so when it fails (oom can happen), it'll crash.
Assignee | ||
Updated•16 years ago
|
Product: Core → MailNews Core
Updated•15 years ago
|
Summary: fSearchResults = nsImapSearchResultSequence::CreateSearchResultSequence() isn't null checked → crash possible because fSearchResults = nsImapSearchResultSequence::CreateSearchResultSequence() isn't null checked
Comment 1•14 years ago
|
||
comment 0 still appears to be true
(checking vers=1.8/2.0 crash bugs)
Version: 1.8 Branch → Trunk
Comment 2•14 years ago
|
||
http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapSearchResults.h
42 class nsImapSearchResultSequence : public nsVoidArray
44 public:
46 static nsImapSearchResultSequence *CreateSearchResultSequence();
53 private:
54 nsImapSearchResultSequence();
http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapSearchResults.cpp#48
49 nsImapSearchResultSequence *nsImapSearchResultSequence::CreateSearchResultSequence()
50 {
51 return new nsImapSearchResultSequence;
52 }
So this is a "trivial" factory, isn't it?
***
http://mxr.mozilla.org/comm-central/search?string=CreateSearchResultSequence&case=1&find=%2Fmailnews%2Fimap%2Fsrc%2F
/mailnews/imap/src/nsImapServerResponseParser.cpp
* line 83 -- fSearchResults = nsImapSearchResultSequence::CreateSearchResultSequence();
/mailnews/imap/src/nsImapSearchResults.h
* line 46 -- static nsImapSearchResultSequence *CreateSearchResultSequence();
/mailnews/imap/src/nsImapSearchResults.cpp
* line 49 -- nsImapSearchResultSequence *nsImapSearchResultSequence::CreateSearchResultSequence()
And, ftr, it's called in one place only.
*****
http://en.wikipedia.org/wiki/New_(C%2B%2B)
"if new can not allocate memory on the heap it will throw an exception of type std::bad_alloc"
https://developer.mozilla.org/en/Infallible_memory_allocation
"Infallible memory allocation
Introduced in Gecko 2.0"
http://mxr.mozilla.org/mozilla-central/source/memory/mozalloc/mozalloc.h
I'm not familiar with Gecko memory allocations (in trunk and branches).
Is a try+catch missing? (It doesn't seem to be the way Mozilla does it :-|)
Is this implicitly using a fallible allocator which can return null instead of throwing?
...
prior to mozilla2 our code was written using a design and compiler flags such that new returned null on failure instead of throwing an exception.
This enabled us to theoretically write applications which didn't abort when low on memory.
The tradeoff for this is that each place where we allocated memory we had to ensure we checked for failure. If we didn't check for failure, scary things could happen.
With mozilla2 we're trying to switch to the more "standard" behavior where new causes the app to die when you run out of memory. this is theoretically safer form a security perspective, but it does mean any data which is in an unhappy state or uncommitted state will be left that way (or lost).
Comment 4•14 years ago
|
||
(In reply to comment #3)
Thanks for the explanation/confirmation.
***
Then,
on 2.0, this would be WontFix or it should call the fallible allocator;
on 1.9.2/1.9.1, we could add a null-check(s) or leave it WontFix.
Comment 6•10 years ago
|
||
(In reply to :aceman from comment #5)
> So is this still relevant?
magnus can you tell? (if not please pass on to rkent or irving)
Assignee: mozilla → nobody
Flags: needinfo?(mkmelin+mozilla)
Reporter | ||
Comment 7•10 years ago
|
||
I don't know. Passing the question along
Flags: needinfo?(mkmelin+mozilla) → needinfo?(irving)
Comment 8•10 years ago
|
||
Looks to me like we're using the default allocator, so it should throw an exception when out of memory. We'd have to pass the OOM handling up through the nsImapServerResponseParser constructor, which I don't think is worth doing at this point.
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: needinfo?(irving)
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•