Closed
Bug 307537
Opened 19 years ago
Closed 19 years ago
Crash on cursor motion in text entry due to overrun of clusterBuffer
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
VERIFIED
FIXED
People
(Reporter: iwj, Assigned: MatsPalmgren_bugz)
References
()
Details
(Keywords: crash, verified1.8, Whiteboard: [sg:fix])
Attachments
(2 files)
8.44 KB,
patch
|
dbaron
:
review+
dbaron
:
superreview+
dbaron
:
approval1.8b5+
|
Details | Diff | Splinter Review |
5.83 KB,
patch
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.8) Gecko/20050513 Debian/1.7.8-1 Build Identifier: firefox (1.0.6-1ubuntu10) breezy and others Bug was originally reported to ubuntu in our patched 1.0.6, as http://bugzilla.ubuntu.com/buglist.cgi?cmdtype=runnamed&namedcmd=Todo1 I have checked the source from Debian's archive and the bug is also in mozilla-firefox_1.0.99+deerpark-alpha2.orig.tar.gz which I assume is the same as your original upstream tarball. Reproducible: Always Steps to Reproduce: 1. Visit the provided URL. 2. Left-click somewhere in the middle of the text in the final text box. 3. Press the left arrow key. Actual Results: Firefox takes a segmentation fault. Expected Results: Moved the cursor left. This patch fixes the problem, and I believe it to be correct: --- orig/firefox-1.0.6/layout/html/base/src/nsTextFrame.cpp 2005-09-08 19:11:43.000000000 +0100 +++ firefox-1.0.6/layout/html/base/src/nsTextFrame.cpp 2005-09-08 18:07:18.000000000 +0100 @@ -4008,12 +4008,6 @@ } PRInt32* ip = indexBuffer.mBuffer; - nsAutoIndexBuffer clusterBuffer; - rv = clusterBuffer.GrowTo(mContentLength + 1); - if (NS_FAILED(rv)) { - return rv; - } - PRInt32 textLength; nsresult result(NS_ERROR_FAILURE); aPos->mResultContent = mContent;//do this right off @@ -4055,6 +4049,12 @@ nsTextTransformer tx(doc->GetLineBreaker(), nsnull, aPresContext); PrepareUnicodeText(tx, &indexBuffer, &paintBuffer, &textLength); + nsAutoIndexBuffer clusterBuffer; + rv = clusterBuffer.GrowTo(textLength + 1); + if (NS_FAILED(rv)) { + return rv; + } + nsIFrame *frameUsed = nsnull; PRInt32 start; PRBool found = PR_TRUE;
Comment 1•19 years ago
|
||
Could you provide a talkback ID for the crash? We might already have another bug opened for this. Also, WFM in Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20050907 Firefox/1.6a1 ID:2005090711
Component: General → Layout
Product: Firefox → Core
QA Contact: general → layout
Version: unspecified → 1.7 Branch
Assignee | ||
Comment 2•19 years ago
|
||
Reproduced in a trunk debug build on Linux. Patch coming up...
Assignee: nobody → mats.palmgren
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Hardware: PC → All
Version: 1.7 Branch → Trunk
Assignee | ||
Updated•19 years ago
|
Flags: blocking1.8b5?
Flags: blocking1.7.12?
Flags: blocking-aviary1.0.7?
Assignee | ||
Comment 3•19 years ago
|
||
1. make FillClusterBuffer() grow the result buffer as needed 2. fix OOM crasher on CreateRenderingContext() and propagate the result code from GetClusterInfo() 3. remove 4 unused variables that the compiler warns about
Attachment #195591 -
Flags: superreview?(dbaron)
Attachment #195591 -
Flags: review?(dbaron)
Comment on attachment 195591 [details] [diff] [review] Patch rev. 1 This seems to be for pango-enabled builds only, right, since they're the only ones that implement GetClusterInfo. >+ nsresult rv = aClusterBuffer.GrowTo(aLength + 1); It looks (from nsIRenderingContext and the pango GetClusterInfo implementation) like this only needs to be |aLength|. Do double-check that, though. ;-) >- shell->CreateRenderingContext(this, getter_AddRefs(acx)); >+ rv = shell->CreateRenderingContext(this, getter_AddRefs(acx)); >+ NS_ENSURE_SUCCESS(rv, rv); > > // Find the font metrics for this text > SetFontFromStyle(acx, mStyleContext); > >- if (acx) >- acx->GetHints(clusterHint); >+ acx->GetHints(clusterHint); It might be worth double-checking that CreateRenderingContext implementations don't return NS_OK and a null pointer. r+sr=dbaron
Attachment #195591 -
Flags: superreview?(dbaron)
Attachment #195591 -
Flags: superreview+
Attachment #195591 -
Flags: review?(dbaron)
Attachment #195591 -
Flags: review+
Assignee | ||
Comment 5•19 years ago
|
||
(In reply to comment #4) > (From update of attachment 195591 [details] [diff] [review] [edit]) > This seems to be for pango-enabled builds only, right, since they're > the only ones that implement GetClusterInfo. The crash is for all builds. The potential exploit is probably only for platforms that implement GetClusterInfo() since otherwise we will overwrite with 1's (one or more) which seems more difficult to use... http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsTextFrame.cpp&rev=1.520&root=/cvsroot&mark=1673#1650 > >+ nsresult rv = aClusterBuffer.GrowTo(aLength + 1); > > It looks ... like this only needs to be |aLength|. Agreed. BTW, looking at nsFontMetricsPango.cpp it doesn't seem to cope with embedded NULs, this looks like an UMR on attrs[] for example: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/gfx/src/gtk/nsFontMetricsPango.cpp&rev=1.18&root=/cvsroot&mark=904,916,923,929#893 > > // Find the font metrics for this text > > SetFontFromStyle(acx, mStyleContext); > > > >- if (acx) > >- acx->GetHints(clusterHint); > >+ acx->GetHints(clusterHint); > > It might be worth double-checking that CreateRenderingContext implementations > don't return NS_OK and a null pointer. We wouldn't reach that line anyway because SetFontFromStyle() would have already crashed nicely for us ;-) http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsFrame.cpp&rev=3.587&root=/cvsroot&mark=434#427 I checked all CreateRenderingContext() and they seemed ok.
Keywords: crash
Assignee | ||
Comment 6•19 years ago
|
||
Removed the '+ 1' and checked in to trunk at 2005-09-11 18:05 PDT
Assignee | ||
Updated•19 years ago
|
Attachment #195591 -
Flags: approval1.8b5?
Attachment #195591 -
Flags: approval1.7.12?
Attachment #195591 -
Flags: approval-aviary1.0.7?
Updated•19 years ago
|
Group: security
Updated•19 years ago
|
Flags: blocking1.7.12?
Flags: blocking-aviary1.0.7?
Whiteboard: [sg:fix]
Attachment #195591 -
Flags: approval1.8b5? → approval1.8b5+
Updated•19 years ago
|
Flags: blocking1.8b5? → blocking1.8b5+
Assignee | ||
Comment 7•19 years ago
|
||
This patch includes the '+ 1' removal and the "unused variable" compile warning cleanups is not included in this version since those did not exist on branch. Checked in to MOZILLA_1_8_BRANCH at 2005-09-13 21:21 PDT -> FIXED
Assignee | ||
Updated•19 years ago
|
Updated•19 years ago
|
Status: RESOLVED → VERIFIED
Keywords: fixed1.8 → verified1.8
Updated•19 years ago
|
Flags: blocking1.7.13?
Flags: blocking1.7.13+
Flags: blocking1.7.12?
Flags: blocking-aviary1.0.8?
Flags: blocking-aviary1.0.8+
Flags: blocking-aviary1.0.7?
Reporter | ||
Comment 8•19 years ago
|
||
Can this bug now not be made public now ? The Debian firefox maintainer would like to see it ...
Updated•19 years ago
|
Flags: testcase+
Comment 9•19 years ago
|
||
Comment on attachment 196005 [details] [diff] [review] Final patch for 1.8 branch a=dveditz for drivers, please add fixed-aviary1.0.8 and fixed1.7.13 keywords when checked in.
Attachment #196005 -
Flags: approval1.7.13+
Attachment #196005 -
Flags: approval-aviary1.0.8+
Updated•19 years ago
|
Attachment #195591 -
Flags: approval1.7.13?
Attachment #195591 -
Flags: approval-aviary1.0.8?
Assignee | ||
Comment 10•19 years ago
|
||
Comment on attachment 196005 [details] [diff] [review] Final patch for 1.8 branch This patch is not needed on the older branches. I was about to apply the patch to check it in but discovered that the FillClusterBuffer method does not exist. It was first added by bug 260663 so I don't think 1.7.13/1.0.8 is affected by this bug. I did a build from the MOZILLA_1_7_BRANCH and verified I didn't crash on the URL.
Attachment #196005 -
Attachment description: Final patch for 1.8 branch (use this for older branches too) → Final patch for 1.8 branch
Attachment #196005 -
Flags: approval1.7.13+
Attachment #196005 -
Flags: approval-aviary1.0.8+
Comment 11•19 years ago
|
||
Removing blocking flags from the old branches
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8+
Updated•17 years ago
|
Group: security
Updated•17 years ago
|
Flags: in-testsuite+ → in-testsuite?
You need to log in
before you can comment on or make changes to this bug.
Description
•