Last Comment Bug 506267 - nsTableCellMap::Synchronize misplaced map
: nsTableCellMap::Synchronize misplaced map
Status: RESOLVED FIXED
: [sg:moderate]
: coverity, fixed1.9.0.16, testcase-wanted
Product: Core
Classification: Components
Component: Layout: Tables
: Trunk
: All All
: P2 critical (vote)
: mozilla1.9.3a1
Assigned To: Reed Loden [:reed] (very busy)
: layout.tables
: http://mxr.mozilla.org/mozilla-centra...
:
:
  Show dependency treegraph
 
Reported: 2009-07-24 08:21 PDT by timeless
Modified: 2010-04-01 20:18 PDT (History)
8 users (show)
dbaron: blocking1.9.2+
dbaron: wanted1.9.2+
dveditz: blocking1.9.0.16+
dveditz: wanted1.9.0.x+
See Also:
Crash Signature:
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  beta1-fixed
  .6+
  .6-fixed


Attachments
patch - v1 (untested) (795 bytes, patch)
2009-09-14 08:17 PDT, Reed Loden [:reed] (very busy)
bzbarsky: review+
dbaron: superreview+
Details | Diff | Splinter Review
patch - v2 (881 bytes, patch)
2009-10-02 18:49 PDT, Reed Loden [:reed] (very busy)
reed: review+
reed: superreview+
roc: approval1.9.2+
dveditz: approval1.9.1.6+
dveditz: approval1.9.0.16+
Details | Diff | Splinter Review

Summon comment box

Description timeless 2009-07-24 08:21:58 PDT
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 2009-07-24 08:48:20 PDT
yep, we should probably break once we need to delete. Feeding null again seems like adding more injury.
Comment 2 Reed Loden [:reed] (very busy) 2009-09-14 08:17:04 PDT
Created attachment 400509 [details] [review]
patch - v1 (untested)

Something as simple as this?
Comment 3 Reed Loden [:reed] (very busy) 2009-10-02 17:42:33 PDT
I see this same code all the way back to at least 1.9.0.
Comment 4 David Baron [:dbaron] 2009-10-02 17:53:16 PDT
Why is this a security bug?  It looks like a performance improvement for out-of-memory handling to me.
Comment 5 David Baron [:dbaron] 2009-10-02 17:53:48 PDT
Oh, never mind.
Comment 6 David Baron [:dbaron] 2009-10-02 17:54:22 PDT
Comment on attachment 400509 [details] [review]
patch - v1 (untested)

sr=dbaron
Comment 7 David Baron [:dbaron] 2009-10-02 17:54:56 PDT
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] (very busy) 2009-10-02 18:49:36 PDT
Created attachment 404381 [details] [review]
patch - v2

Yeah, good idea.
Comment 9 Reed Loden [:reed] (very busy) 2009-10-02 19:11:26 PDT
http://hg.mozilla.org/mozilla-central/rev/05d91121cbc7
Comment 10 Daniel Veditz 2009-10-04 15:30:49 PDT
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] (very busy) 2009-10-07 08:25:07 PDT
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/759d02e54215
Comment 12 Samuel Sidler (old account; do not CC) 2009-10-16 10:26:26 PDT
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] 2009-10-16 10:28:39 PDT
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] 2009-10-16 10:29:30 PDT
Were test builds made with this patch on 1.9.1 or 1.9.0?
Comment 15 Samuel Sidler (old account; do not CC) 2009-10-19 14:46:39 PDT
Comment on attachment 404381 [details] [review]
patch - v2

Clearing approval requests until comments 12-14 are answered.
Comment 16 Reed Loden [:reed] (very busy) 2009-10-19 14:58:49 PDT
(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] 2009-10-19 15:10:17 PDT
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] (very busy) 2009-10-19 15:19:11 PDT
(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] 2009-10-19 15:59:27 PDT
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] 2009-10-19 16:03:29 PDT
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] (very busy) 2009-10-20 00:30:40 PDT
Comment on attachment 404381 [details] [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 2009-10-23 10:38:49 PDT
Comment on attachment 404381 [details] [review]
patch - v2

Approved for 1.9.1.5 and 1.9.0.16, a=dveditz for release-drivers
Comment 23 Reed Loden [:reed] (very busy) 2009-10-23 11:37:35 PDT
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2b9f6f5f5d18
Comment 24 Reed Loden [:reed] (very busy) 2009-10-23 11:48:49 PDT
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] 2009-11-10 16:21:54 PST
Nothing for QA to do here since we have no way to test this fix.

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