code blob: 284 // Scope |map| outside the loop so we can use it as a hint. 285 nsCellMap* map = nsnull; 286 for (PRUint32 rgX = 0; rgX < orderedRowGroups.Length(); rgX++) { 287 nsTableRowGroupFrame* rgFrame = orderedRowGroups[rgX]; 288 map = GetMapFor((nsTableRowGroupFrame*)rgFrame->GetFirstInFlow(), map); 289 if (map) { 290 if (!maps.AppendElement(map)) { 291 delete map; 292 NS_WARNING("Could not AppendElement"); 293 } 294 } 295 } logic flow: 285 nsCellMap* map = nsnull; 286 for (PRUint32 rgX = 0; rgX < orderedRowGroups.Length(); rgX++) { 288 map = GetMapFor((nsTableRowGroupFrame*)rgFrame->GetFirstInFlow(), map); 289 if (map) { 290 if (!maps.AppendElement(map)) { 291 delete map; 286 for (PRUint32 rgX = 0; rgX < orderedRowGroups.Length(); rgX++) { 288 map = GetMapFor((nsTableRowGroupFrame*)rgFrame->GetFirstInFlow(), map);

yep, we should probably break once we need to delete. Feeding null again seems like adding more injury.

Something as simple as this?

I see this same code all the way back to at least 1.9.0.

Why is this a security bug? It looks like a performance improvement for out-of-memory handling to me.

Oh, never mind.

Comment on attachment 400509 [details] [diff] [review] patch - v1 (untested) sr=dbaron

Though, it seems even better to set map to null right after deleting it (possibly in addition to this change).

Yeah, good idea.

Is there any reason other than OOM that maps.AppendElement() would fail? use-after-free can be exploitable, but lowering to "sg:moderate": the timing required to run us out of memory at just the right time to get here seems like a pretty hard thing to turn into a reliably reproducible exploit. QA will want a testcase to verify this if possible, though maybe it's not. If we can come up with a testcase that reliably crashes here (in a debug build, say, or using MallocScribble or gflags) that'd justify putting it back to critical. This doesn't need to block the current already-late stable branch releases.

How was this tested? Are we sure it builds on 1.9.1 and 1.9.0? It's not clear to me that just because you "see this same code all the way back to at least 1.9.0" that it actually applies in the same way. Also, we really need a testcase here. Who's working on creating one?

How exactly is QA supposed to verify this fix? We need either/or a manual testcase and an automated testcase (to be added when this bug is made public). Otherwise, we're just taking it on faith that it fixes the issue.

Were test builds made with this patch on 1.9.1 or 1.9.0?

Comment on attachment 404381 [details] [diff] [review] patch - v2 Clearing approval requests until comments 12-14 are answered.

(In reply to comment #12) > How was this tested? Are we sure it builds on 1.9.1 and 1.9.0? It's not clear > to me that just because you "see this same code all the way back to at least > 1.9.0" that it actually applies in the same way. I am sure it builds on 1.9.1 and 1.9.0, as that code is exactly the same. > Also, we really need a testcase here. Who's working on creating one? Not me. (In reply to comment #13) > How exactly is QA supposed to verify this fix? We need either/or a manual > testcase and an automated testcase (to be added when this bug is made public). > Otherwise, we're just taking it on faith that it fixes the issue. It's not "taking it on faith" if you can read this code, see the problem, and see the obvious fix. However, I will dig through coverity to get the CID and make sure coverity has marked it as fixed. That's the best I can do. Also, note dveditz's comment #10 where a testcase for this bug may not really be feasible. (In reply to comment #14) > Were test builds made with this patch on 1.9.1 or 1.9.0? No, I can throw the patch at the try server to generate some, if you really want them, but not sure what that's going to prove or actually do for you.

Well, the reason you are being asked questions is violated explicit process (by not building on try server first to make sure your code applied correctly) and implicit process (by not making any tests) when you did this patch.

(In reply to comment #17) > Well, the reason you are being asked questions is violated explicit process (by > not building on try server first to make sure your code applied correctly) and > implicit process (by not making any tests) when you did this patch. Wrong and wrong. There is no such process that requires people to use the try server, nor has there ever been. I know many developers who don't use it or who use it rarely. Also, layout doesn't have any set policy that tests are required for every patch. Again, read comment #10. dveditz isn't even sure if a testcase _can_ be written for this.

I don't see any process that's been violated here. Furthermore, I don't think QA verification is useful here, since determining whether the patch fixes this bug tells us nothing about the risk of taking the patch, and I don't think QA verification of the type requested is useful at all, since following a set of steps given by a developer who already took them won't even catch most of the cases where the bug actually isn't fixed, unless such testing is requested by a developer who didn't do testing he or she believed to be needed.

Generally, what I've seen is that a developer does a test build of his or her checkin on Trunk or 1.9.2 and maybe writes a test for it there. That test may or may not be run (whether it is manual or automated) on 1.9.0 or 1.9.1 by the same developer. We've had a number of bugs that regressed on 1.9.0. and 1.9.1 because while the trunk fix applied cleanly, it didn't actually fix the problem on the branches because of the necessity of other patches which were present on trunk but not on the branches. So, you're right, if the developer actually checks that the fix fixes the problem in each place it is applied, QA isn't necessary except for defense in depth. That doesn't seem to happen in many instances though.

Comment on attachment 404381 [details] [diff] [review] patch - v2 (In reply to comment #20) > So, you're right, if the developer actually checks that the fix fixes the > problem in each place it is applied, QA isn't necessary except for defense in > depth. That doesn't seem to happen in many instances though. Considering the code is exactly the same on 1.9.1 and 1.9.0 for what this patch fixes, I'm re-requesting approval. I've been trying to find the original CID this came from, but Coverity's sucky interface isn't making that easy, and I can't seem to find it. Maybe timeless can help? However, I definitely am not seeing this in the most current coverity run, however.

Comment on attachment 404381 [details] [diff] [review] patch - v2 Approved for 1.9.1.5 and 1.9.0.16, a=dveditz for release-drivers

Checking in layout/tables/nsCellMap.cpp; /cvsroot/mozilla/layout/tables/nsCellMap.cpp,v <-- nsCellMap.cpp new revision: 3.145; previous revision: 3.144 done

Nothing for QA to do here since we have no way to test this fix.