527400 - OOM null deref introduced by fix for bug 514872

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Daniel Veditz [:dveditz]]

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 (he/him)]

Attachment: 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 (he/him)]

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]]

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]]

We'll want this on the other branches that got 514872, too.

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

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 6

[:Gijs (he/him)]

http://hg.mozilla.org/mozilla-central/rev/cbaeccb42882

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/13a02fce20e5

Comment 7

[:Gijs (he/him)]

(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: [e-mail]]]

.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 (he/him)]

(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 (he/him)]

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