Last Comment Bug 527400 - OOM null deref introduced by fix for bug 514872
: OOM null deref introduced by fix for bug 514872
Status: RESOLVED FIXED
: crash, fixed1.9.0.18, regression
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: :Gijs Kruitbosch
:
Mentors:
Depends on:
Blocks: 514872
  Show dependency treegraph
 
Reported: 2009-11-08 19:38 PST by Daniel Veditz [:dveditz]
Modified: 2009-12-26 07:48 PST (History)
3 users (show)
dveditz: wanted1.9.0.x+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta5-fixed
.8+
.8-fixed


Attachments
Patch moving the memcpy below the nullcheck (840 bytes, patch)
2009-11-09 03:46 PST, :Gijs Kruitbosch
jwalden+bmo: review+
mbeltzner: approval1.9.2+
mbeltzner: approval1.9.1.8+
mbeltzner: approval1.9.0.18+
Details | Diff | Review

Description Daniel Veditz [:dveditz] 2009-11-08 19:38:00 PST
The patches in bug 514872 fixed the problem of forgetting to copy the expression, but it does so before checking whether the allocation succeeds. If for some reason it fails (and if it never does why are we checking at all?) then we'll crash on a null deref doing memcpy(0,...)

Not likely in practice, and we've talked about making allocation failures instantly fatal anyway, but in the meantime static source checkers like Coverity are going to complain at us.
Comment 1 :Gijs Kruitbosch 2009-11-09 03:46:22 PST
Created attachment 411161 [details] [diff] [review]
Patch moving the memcpy below the nullcheck

I'm really appalled at having missed the one thing there possibly was to miss about this fix... Anyway, let's get this done. :-\
Comment 2 :Gijs Kruitbosch 2009-11-23 12:34:54 PST
Blah, I dropped the ball on this - sorry! The tree is currently restricted - should we attempt to still get this for 3.6? It's pretty harmless, as far as I can tell... dveditz, what do you reckon?
Comment 3 Daniel Veditz [:dveditz] 2009-11-30 22:11:10 PST
Comment on attachment 411161 [details] [diff] [review]
Patch moving the memcpy below the nullcheck

Doesn't block 1.9.2, but is wanted on that branch and safe to take.
Comment 4 Daniel Veditz [:dveditz] 2009-11-30 22:13:16 PST
We'll want this on the other branches that got 514872, too.
Comment 5 Mike Beltzner [:beltzner, not reading bugmail] 2009-12-02 05:10:42 PST
Comment on attachment 411161 [details] [diff] [review]
Patch moving the memcpy below the nullcheck

a=beltzner for all branches, please land on 1917 and 19017 when the builds are done for 1916 and 19016
Comment 7 :Gijs Kruitbosch 2009-12-24 07:23:11 PST
(In reply to comment #5)
> (From update of attachment 411161 [details] [diff] [review])
> a=beltzner for all branches, please land on 1917 and 19017 when the builds are
> done for 1916 and 19016

Gah, missed that train (well, .7beta is out, according to the testing announcement in .planning). From the announcement there of .6 being released (the 16th) there was an 8-day window to check this in - but you wrote "when the builds are done", not when the release is announced - is there a better thing than the release announcement I should be looking for in terms of the .7 builds being Done so I can land this for .8 ?

Also, this bug was originally marked blocking 1.9.1.7+, and is now .8+, but there was no comment on this bug about it being pushed back (or asking me to land it soon please because...). I'm not trying to shift blame here, but I'm also not really clear on how this blocking mechanism then works. :-)
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-12-24 10:39:16 PST
.7 and .17 were pushed forward for a few specific fixes, and are being shipped from the previous relbranch - this bug specifically wasn't pushed back. You're free to land on 1.9.0/1.9.1 now for 1.9.0.18/1.9.1.8.
Comment 9 :Gijs Kruitbosch 2009-12-24 11:04:32 PST
(In reply to comment #8)
> .7 and .17 were pushed forward for a few specific fixes, and are being shipped
> from the previous relbranch - this bug specifically wasn't pushed back. You're
> free to land on 1.9.0/1.9.1 now for 1.9.0.18/1.9.1.8.

OK, great, thanks for clarifying! Will push those changes as soon as I can get to it inbetween the goings-on these days... Merry Christmas! :-)
Comment 10 :Gijs Kruitbosch 2009-12-26 07:48:37 PST
Checking in nsWildCard.cpp;
/cvsroot/mozilla/xpfe/components/filepicker/src/nsWildCard.cpp,v  <--  nsWildCard.cpp
new revision: 1.7; previous revision: 1.6
done


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/076725c55412

Note You need to log in before you can comment on or make changes to this bug.