Closed Bug 506267 Opened 11 years ago Closed 11 years ago

nsTableCellMap::Synchronize misplaced map

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1
Tracking Status
status1.9.2 --- beta1-fixed
blocking1.9.1 --- .6+
status1.9.1 --- .6-fixed

People

(Reporter: timeless, Assigned: reed)

References

(Blocks 1 open bug, )

Details

(Keywords: coverity, fixed1.9.0.16, Whiteboard: [sg:moderate])

Attachments

(1 file, 1 obsolete file)

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.
Attached patch patch - v1 (untested) (obsolete) — Splinter Review
Something as simple as this?
Attachment #400509 - Flags: review?
Attachment #400509 - Flags: review? → review?(bernd_mozilla)
Attachment #400509 - Flags: review?(bzbarsky)
Attachment #400509 - Flags: review?(bzbarsky)
Attachment #400509 - Flags: review?(bernd_mozilla)
Attachment #400509 - Flags: review+
Assignee: bernd_mozilla → reed
Attachment #400509 - Flags: review+ → superreview?(dbaron)
I see this same code all the way back to at least 1.9.0.
Status: NEW → ASSIGNED
blocking1.9.1: --- → ?
status1.9.1: --- → ?
Flags: wanted1.9.0.x?
Flags: blocking1.9.2?
Flags: blocking1.9.0.16?
Flags: blocking1.9.0.15?
Whiteboard: [sg:critical?]
Why is this a security bug?  It looks like a performance improvement for out-of-memory handling to me.
Comment on attachment 400509 [details] [diff] [review]
patch - v1 (untested)

sr=dbaron
Attachment #400509 - Flags: superreview?(dbaron) → superreview+
Though, it seems even better to set map to null right after deleting it (possibly in addition to this change).
Attached patch patch - v2Splinter Review
Yeah, good idea.
Attachment #400509 - Attachment is obsolete: true
Attachment #404381 - Flags: superreview+
Attachment #404381 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/05d91121cbc7
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Attachment #404381 - Flags: approval1.9.2?
Attachment #404381 - Flags: approval1.9.1.5?
Attachment #404381 - Flags: approval1.9.1.4?
Attachment #404381 - Flags: approval1.9.0.16?
Attachment #404381 - Flags: approval1.9.0.15?
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.
blocking1.9.1: ? → needed
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.15?
Keywords: testcase-wanted
Whiteboard: [sg:critical?] → [sg:moderate]
Attachment #404381 - Flags: approval1.9.1.4?
Attachment #404381 - Flags: approval1.9.0.15?
Attachment #404381 - Flags: approval1.9.2? → approval1.9.2+
Flags: wanted1.9.2+
Flags: blocking1.9.2?
Flags: blocking1.9.2-
Flags: blocking1.9.2- → blocking1.9.2+
Priority: -- → P2
blocking1.9.1: needed → .5+
Flags: blocking1.9.0.16? → blocking1.9.0.16+
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?
Attachment #404381 - Flags: approval1.9.1.5?
Attachment #404381 - Flags: approval1.9.0.16?
Comment on attachment 404381 [details] [diff] [review]
patch - v2

Clearing approval requests until comments 12-14 are answered.
Whiteboard: [sg:moderate] → [sg:moderate][needs answer to comments 12-14]
(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.
Whiteboard: [sg:moderate][needs answer to comments 12-14] → [sg:moderate]
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.
Attachment #404381 - Flags: approval1.9.1.5?
Attachment #404381 - Flags: approval1.9.0.16?
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
Attachment #404381 - Flags: approval1.9.1.5?
Attachment #404381 - Flags: approval1.9.1.5+
Attachment #404381 - Flags: approval1.9.0.16?
Attachment #404381 - Flags: approval1.9.0.16+
Component: Layout → Layout: Tables
QA Contact: layout → layout.tables
Checking in layout/tables/nsCellMap.cpp;
/cvsroot/mozilla/layout/tables/nsCellMap.cpp,v  <--  nsCellMap.cpp
new revision: 3.145; previous revision: 3.144
done
Keywords: fixed1.9.0.16
Nothing for QA to do here since we have no way to test this fix.
Group: core-security
You need to log in before you can comment on or make changes to this bug.