Last Comment Bug 489647 - (CVE-2009-1313) New 1.9.0.9 topcrash [@nsTextFrame::ClearTextRun()]
(CVE-2009-1313)
: New 1.9.0.9 topcrash [@nsTextFrame::ClearTextRun()]
Status: VERIFIED FIXED
[sg:critical?]
: crash, regression, topcrash, verified1.9.0.10, verified1.9.0.11
Product: Core
Classification: Components
Component: Layout (show other bugs)
: 1.9.0 Branch
: All All
: -- critical (vote)
: ---
Assigned To: Daniel Holbert [:dholbert]
:
:
Mentors:
: 489676 490233 (view as bug list)
Depends on: 467150 490410
Blocks: 431260 489322
  Show dependency treegraph
 
Reported: 2009-04-22 13:06 PDT by Daniel Veditz [:dveditz]
Modified: 2011-06-13 10:01 PDT (History)
20 users (show)
dveditz: blocking1.9.0.11+
dveditz: wanted1.9.0.x+
asac: wanted1.8.1.x-
asac: wanted1.8.0.x-
dholbert: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
'revised patch' from bug 467150, backported to CVS trunk (3.63 KB, patch)
2009-04-22 22:58 PDT, Daniel Holbert [:dholbert]
roc: review+
samuel.sidler+old: approval1.9.0.10+
dveditz: approval1.9.0.11+
Details | Diff | Splinter Review
testcase (289 bytes, text/html)
2009-04-23 03:46 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
Patch to add crashtest to trunk (998 bytes, patch)
2009-05-07 12:58 PDT, cmtalbert
dholbert: review+
Details | Diff | Splinter Review
Patch for 1.9.1 (998 bytes, patch)
2009-05-07 13:06 PDT, cmtalbert
dholbert: review+
Details | Diff | Splinter Review
Patch for 1.9.0 (1.32 KB, patch)
2009-05-07 13:25 PDT, cmtalbert
dholbert: review+
Details | Diff | Splinter Review

Description Daniel Veditz [:dveditz] 2009-04-22 13:06:09 PDT
Firefox 3.0.9 has exposed a new topcrash @nsTextFrame::ClearTextRun(). One instance is bug 489322, involving the HTML Validator addon. We'll see soon if that's the only case because the author of that addon is about to release an update with a workaround (disables the crashing feature).

Filing separate to track as a security bug.

Crashes:
http://crash-stats.mozilla.com/report/list?product=Firefox&version=Firefox%3A3.0.9&query_search=signature&query_type=exact&query=&date=&range_value=1&range_unit=weeks&do_query=1&signature=nsTextFrame%3A%3AClearTextRun()

This appears to be exploitable given random addresses at the top of the stack
bp-e5e76111-98f2-4785-9fe6-ba0582090421
bp-49a91d2b-b49c-4316-957e-d2c9b2090421
bp-87a98e87-4982-488f-8c11-6a2c72090421
bp-043a79b6-250a-4d52-8862-ef1d72090421
etc.

Here's one with a comment that does NOT mention view source -- this one says they did a ctrl-f (find) and clicked the "Highlight All" button.
bp-d31d7a90-09b4-4cf5-9baf-7b1952090422
Comment 1 Samuel Sidler (old account; do not CC) 2009-04-22 13:15:56 PDT
Regression range is between the 2009-02-26 and 2009-02-27 builds on Windows only (looks like the extension has specific versions per-OS).

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2009-02-26&maxdate=2009-02-27&cvsroot=%2Fcvsroot

Pretty sure this is either bug 431260 or bug 444027.
Comment 2 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2009-04-22 13:31:37 PDT
Given the build hour being 06, you probably want a range more like http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2009-02-26%2004:00&maxdate=2009-02-27%2007:00&cvsroot=%2Fcvsroot although you could reduce it by looking at tinderbox.

In any case, I'd bet on bug 431260.
Comment 3 Daniel Holbert [:dholbert] 2009-04-22 14:03:05 PDT
Changing OS field from Mac to Windows, since the crash report table shows 0 mac crashes, 1 linux crash, and 1113 windows crashes.  (We can change the field to "All" after confirming a crash on another platform)

Note: Bugzilla's auto-linking doesn't catch the "()" on the end of the first URL in comment 0 (the table of all matching crash reports) -- you have to copy and paste the full link (or click it and then manually add "()" at the end) in order for the link to work.
Comment 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-04-22 14:58:43 PDT
So we fixed 431260, which wasn't really a security problem, and we introduced this bug which probably is. Perhaps we need to be more picky about what we land on branch.
Comment 5 Samuel Sidler (old account; do not CC) 2009-04-22 15:50:43 PDT
Daniel: Giving this over to you since you fixed bug 431260.
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-04-22 16:27:16 PDT
Since this isn't a problem on trunk or 1.9.1 (I presume), it's almost certain that a change in 1.9.1 fixed or prevented this bug. We just need to figure out which one.

Bug 472776 is a strong candidate, Daniel, can you see if that patch fixes your Linux 1.9.0 opt build?
Comment 7 Daniel Holbert [:dholbert] 2009-04-22 16:32:57 PDT
(In reply to comment #6)
> Since this isn't a problem on trunk or 1.9.1 (I presume)

That's correct -- I don't crash in my mozilla-central nightly, whereas I do in a 1.9.0.x nightly (per bug 489322 comment 24).

> Bug 472776 is a strong candidate, Daniel, can you see if that patch fixes your
> Linux 1.9.0 opt build?

Yeah, I'll give that a shot.
Comment 8 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-04-22 16:35:23 PDT
I guess a surefire way to track this would be to take the patch in bug 431260 and bisect along the 1.9.1 branch between the 1.9.0 branch point and where the patch actually landed to identify where adding that patch to the 1.9.1 branch no longer causes a crash.

Thanks Daniel!
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-04-22 16:38:36 PDT
It might not be bug 472776 that fixed it, since that landed on 1.9.1 on Feb 26, months after the patches in bug 431260 landed on 1.9.1.
Comment 10 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-04-22 17:00:40 PDT
Bug 467150 is another possibility.
Comment 11 Daniel Holbert [:dholbert] 2009-04-22 17:27:28 PDT
So while waiting for my opt build to finish, I figured I'd test mozilla-central nightlies from right after the potential regressing patches landed, in the hopes that we crashed there too (and then we'd be able to do a binary-search for the fix range).

I tried these mozilla-central nightly builds, to catch the just-landed fixes for bug 431341, bug 431260 / bug 455826, bug 444027 respectively:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1a2pre) Gecko/2008073002 Minefield/3.1a2pre
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b3pre) Gecko/20081128 Minefield/3.1b3pre
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090211 Minefield/3.2a1pre

However, none of them crashed.  So unless I'm making a mistake, it looks like we never actually hit this crash on mozilla-central. :-/
Comment 12 Daniel Holbert [:dholbert] 2009-04-22 22:58:16 PDT
Created attachment 374220 [details] [diff] [review]
'revised patch' from bug 467150, backported to CVS trunk

(In reply to comment #10)
> Bug 467150 is another possibility.

That bug's patch indeed fixes this (at least, it fixes bug 489322), in my 190x opt build!  I'm attaching a backported version of it here.

It looks like 190x already includes the *first* patch on that bug (labeled "patch that landed on trunk"), whereas mozilla-central has the *second* patch (labeled "revised patch").  The two patches conflict, so this backport simply undoes the first patch and applies the second patch, making 1.9.0.x match mozilla-central.
Comment 13 Daniel Holbert [:dholbert] 2009-04-22 23:17:04 PDT
(In reply to comment #12)
> It looks like 190x already includes the *first* patch on that bug (labeled
> "patch that landed on trunk")

So, it turns out that patch was already applied on trunk because it was actually originally *part of* bug 455826's patch, attachment 348661 [details] [diff] [review], as posted on bugzilla (which I landed in 1.9.0.9 to fix bug 431260).

However, when bug 455826 originally landed on mozilla-central, it was missing that exact chunk due to a minor oversight, as explained in bug 455826 comment 16. That comment explained that this chunk would be handled & land in a separate bug -- bug 467150.  And *that* bug is where this chunk was landed, backed out, and then *improved* to form the patch that ended up actually fixing **this** bug here.

Hope that makes sense.  My bad for not seeing that bug 455826's patch got split up and improved elsewhere -- sorry about that!  I think this all makes sense now, though -- we just need to "complete" the patch for bug 455826 by updating the chunk that was improved in bug 467150.
Comment 14 Daniel Holbert [:dholbert] 2009-04-22 23:20:13 PDT
Comment on attachment 374220 [details] [diff] [review]
'revised patch' from bug 467150, backported to CVS trunk

(In reply to comment #13)
> I think this all makes sense
> now, though -- we just need to "complete" the patch for bug 455826 by updating
> the chunk that was improved in bug 467150.

(That is to say -- we just need to land "patch for bug 467150, backported to CVS trunk", a.k.a.
attachment 374220 [details] [diff] [review])
Comment 15 Daniel Holbert [:dholbert] 2009-04-23 00:12:28 PDT
Comment on attachment 374220 [details] [diff] [review]
'revised patch' from bug 467150, backported to CVS trunk

roc, just to confirm -- as the author of the patch on bug 467150, do you think it's suitable/safe for 1.9.0.x branch?

FWIW, it doesn't seem to have caused any known regressions -- the only dependency on it (besides this bug here and the wikipedia line-wrap bug, both of which I've just added as dependencies) is bug 430332, and that relationship is only there because bug 467150 was part of the fix for bug 430332 (per bug 430332 comment 39).
Comment 16 Martijn Wargers [:mwargers] (not working for Mozilla) 2009-04-23 03:46:11 PDT
Created attachment 374249 [details]
testcase

This is a testcase that seems to reproduce this crash in current 1.9.0.x build:
http://crash-stats.mozilla.com/report/index/6ba4cbb4-9c50-454a-a54a-747f32090423?p=1
Comment 17 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-04-23 03:52:00 PDT
Comment on attachment 374220 [details] [diff] [review]
'revised patch' from bug 467150, backported to CVS trunk

It's my fault for creating such a messy bug structure in the first place.
Comment 18 Daniel Holbert [:dholbert] 2009-04-23 09:48:36 PDT
I've confirmed that the attached patch (attachment 374220 [details] [diff] [review]) fixes the
crash on mw22's testcase (attachment 374249 [details]), in my optimized build.

Testcase crashes immediately before patching & doesn't crash after patching.
Comment 19 Daniel Holbert [:dholbert] 2009-04-23 09:57:40 PDT
I can also confirm the crash with "Hightlight All" mentioned in comment 0 -- the crash report says to load http://msdn.microsoft.com/en-us/library/system.collections.hashtable(VS.71).aspx , hit Ctrl-F & type "using", and click "highlight all" --> crash)

The patch fixes that crash as well.
Comment 20 Daniel Veditz [:dveditz] 2009-04-23 10:12:37 PDT
Comment on attachment 374220 [details] [diff] [review]
'revised patch' from bug 467150, backported to CVS trunk

Approved for 1.9.0.10, a=dveditz for release-drivers
Comment 21 Daniel Holbert [:dholbert] 2009-04-23 10:20:23 PDT
Patch landed:
Checking in layout/generic/nsTextFrameThebes.cpp;
/cvsroot/mozilla/layout/generic/nsTextFrameThebes.cpp,v  <--  nsTextFrameThebes.cpp
new revision: 3.189; previous revision: 3.188
done
Comment 22 Samuel Sidler (old account; do not CC) 2009-04-23 12:30:50 PDT
Comment on attachment 374220 [details] [diff] [review]
'revised patch' from bug 467150, backported to CVS trunk

Approved for 1.9.0.10 (the real one!). a=ss
Comment 23 Samuel Sidler (old account; do not CC) 2009-04-23 12:33:32 PDT
To be clear, my approval is for the GECKO190_20090406_RELBRANCH.
Comment 24 Ben Hearsum (:bhearsum) 2009-04-23 12:35:22 PDT
Comment on attachment 374220 [details] [diff] [review]
'revised patch' from bug 467150, backported to CVS trunk

Landed on the release branch (GECKO190_20090406_RELBRANCH)
Checking in generic/nsTextFrameThebes.cpp;
/cvsroot/mozilla/layout/generic/nsTextFrameThebes.cpp,v  <--  nsTextFrameThebes.cpp
new revision: 3.188.2.1; previous revision: 3.188
done
Comment 25 Daniel Holbert [:dholbert] 2009-04-23 14:32:56 PDT
Setting in-testsuite? flag, as a reminder to check in mw22's testcase once the fix has made it out to users.
Comment 26 Al Billings [:abillings] 2009-04-23 17:04:21 PDT
(In reply to comment #16)
> Created an attachment (id=374249) [details]
> testcase
> 
> This is a testcase that seems to reproduce this crash in current 1.9.0.x build:
> http://crash-stats.mozilla.com/report/index/6ba4cbb4-9c50-454a-a54a-747f32090423?p=1

Verified the crash in 1.9.0.9 and that it no longer occurs in 1.9.0.10 on Linux with  Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.10) Gecko/2009042315 Firefox/3.0.10. 

I'll mark this verified1.9.0.10 after I can check on Windows.
Comment 27 Nick Thomas [:nthomas] 2009-04-24 04:04:33 PDT
*** Bug 489676 has been marked as a duplicate of this bug. ***
Comment 28 Al Billings [:abillings] 2009-04-24 11:52:33 PDT
Verified on Windows XP: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.10) Gecko/2009042316 Firefox/3.0.10 (.NET CLR 3.5.30729)
Comment 29 Nick Thomas [:nthomas] 2009-04-26 12:00:17 PDT
*** Bug 490233 has been marked as a duplicate of this bug. ***
Comment 34 Alexander Sack 2009-04-28 03:28:57 PDT
the code does not exist on 1.8 and the testcase does not trigger a crash there either.
Comment 35 cmtalbert 2009-05-07 12:58:08 PDT
Created attachment 376294 [details] [diff] [review]
Patch to add crashtest to trunk

I just took Martijn's test and made it into a crashtest.  I'll upload patches for 1.9.1 and 1.9.0 as well so that we'll know if we ever regress this.
Comment 36 cmtalbert 2009-05-07 13:06:24 PDT
Created attachment 376297 [details] [diff] [review]
Patch for 1.9.1
Comment 37 Daniel Holbert [:dholbert] 2009-05-07 13:25:05 PDT
Comment on attachment 376294 [details] [diff] [review]
Patch to add crashtest to trunk

Two things:
* Since this test uses setTimeout, could you add "reftest-wait" magic, for correctness?  (Otherwise, if we're super-speedy, I think we could successfully load the test and move on to the next testcase before the setTimeout fires.  Or something like that)

* Also, we should reduce the setTimeout wait-time to 0 (or a small positive value), as long as it still crashes 1.9.0.9 builds, which I think it should.  (We should we can avoid introducing biggish wait-times into our test suite, if at all possible, because they add up.)

Otherwise, looks good!
Comment 38 cmtalbert 2009-05-07 13:25:30 PDT
Created attachment 376301 [details] [diff] [review]
Patch for 1.9.0
Comment 39 Daniel Holbert [:dholbert] 2009-05-07 13:30:18 PDT
Comment on attachment 376294 [details] [diff] [review]
Patch to add crashtest to trunk

One more thing -- looks like the testcase has no newline at end of file (as noted in the raw patch), which is messy -- can you fix that, too?

r=dholbert, after addressing that & comment 37
Comment 40 Daniel Holbert [:dholbert] 2009-05-07 13:31:44 PDT
oh, sorry, I just noticed you have reftest-wait magic already... I don't know how I missed that.  Disregard that part of my comments :)
Comment 41 cmtalbert 2009-05-07 14:58:30 PDT
(In reply to comment #40)
> oh, sorry, I just noticed you have reftest-wait magic already... I don't know
> how I missed that.  Disregard that part of my comments :)

I was about to tell you I already did :) I'll run some tests and see what I can get away with decreasing the setTimeout call to and fix that newline. Thanks for the quick review on this.  I'll repost the final test and carry your review forward.
Comment 42 Al Billings [:abillings] 2009-05-14 17:36:49 PDT
Verified for 1.9.0.11 with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.11pre) Gecko/2009051404 GranParadiso/3.0.11pre.
Comment 43 Daniel Holbert [:dholbert] 2009-10-15 12:28:39 PDT
> (In reply to comment #40)
> I'll run some tests and see what I can
> get away with decreasing the setTimeout call to and fix that newline. Thanks
> for the quick review on this.  I'll repost the final test and carry your review
> forward.

Hi Clint! I think you might've forgotten about this -- looks like this crashtest never got reposted with those fixes (reduced timeout if possible, & add missing newline).
Comment 44 Daniel Holbert [:dholbert] 2009-10-15 12:30:32 PDT
Comment on attachment 376297 [details] [diff] [review]
Patch for 1.9.1

r=dholbert on branch crashtests, with the same fixes from comment 43 that are still needed for the trunk crashtest.

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