506267 - nsTableCellMap::Synchronize misplaced map

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect, P2)

Description

[timeless]

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);

Comment 1

[Bernd]

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

Comment 2

[Reed Loden [:reed]]

Attachment: patch - v1 (untested)

Something as simple as this?

Comment 3

[Reed Loden [:reed]]

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

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oh, never mind.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

sr=dbaron

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 8

[Reed Loden [:reed]]

Attachment: patch - v2

Yeah, good idea.

Comment 9

[Reed Loden [:reed]]

http://hg.mozilla.org/mozilla-central/rev/05d91121cbc7

Comment 10

[Daniel Veditz [:dveditz]]

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.

Comment 11

[Reed Loden [:reed]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/759d02e54215

Comment 12

[Samuel Sidler (old account; do not CC)]

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?

Comment 13

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 14

[Al Billings [:abillings - ex-MoCo]]

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

Comment 15

[Samuel Sidler (old account; do not CC)]

Comment on attachment 404381 [details] [diff] [review]
patch - v2

Clearing approval requests until comments 12-14 are answered.

Comment 16

[Reed Loden [:reed]]

(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.

Comment 17

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 18

[Reed Loden [:reed]]

(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.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 20

[Al Billings [:abillings - ex-MoCo]]

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 21

[Reed Loden [:reed]]

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 22

[Daniel Veditz [:dveditz]]

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

Comment 23

[Reed Loden [:reed]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2b9f6f5f5d18

Comment 24

[Reed Loden [:reed]]

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

Comment 25

[Al Billings [:abillings - ex-MoCo]]

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