If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Crash on cursor motion in text entry due to overrun of clusterBuffer

VERIFIED FIXED

Status

()

Core
Layout
--
critical
VERIFIED FIXED
12 years ago
11 years ago

People

(Reporter: Ian Jackson, Assigned: mats)

Tracking

({crash, verified1.8})

Trunk
crash, verified1.8
Points:
---
Bug Flags:
blocking1.8b5 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:fix], URL)

Attachments

(2 attachments)

(Reporter)

Description

12 years ago
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

12 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

Updated

12 years ago
Component: General → Layout
Product: Firefox → Core
QA Contact: general → layout
Version: unspecified → 1.7 Branch
(Assignee)

Comment 2

12 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

12 years ago
Flags: blocking1.8b5?
Flags: blocking1.7.12?
Flags: blocking-aviary1.0.7?
(Assignee)

Comment 3

12 years ago
Created attachment 195591 [details] [diff] [review]
Patch rev. 1

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

12 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

12 years ago
Removed the '+ 1' and checked in to trunk at 2005-09-11 18:05 PDT
(Assignee)

Updated

12 years ago
Attachment #195591 - Flags: approval1.8b5?
Attachment #195591 - Flags: approval1.7.12?
Attachment #195591 - Flags: approval-aviary1.0.7?
Group: security
Flags: blocking1.7.12?
Flags: blocking-aviary1.0.7?
Whiteboard: [sg:fix]
Attachment #195591 - Flags: approval1.8b5? → approval1.8b5+

Updated

12 years ago
Flags: blocking1.8b5? → blocking1.8b5+
(Assignee)

Comment 7

12 years ago
Created attachment 196005 [details] [diff] [review]
Final patch for 1.8 branch

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

12 years ago
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8
Resolution: --- → FIXED

Updated

12 years ago
Status: RESOLVED → VERIFIED
Keywords: fixed1.8 → verified1.8
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

12 years ago
Can this bug now not be made public now ?  The Debian firefox maintainer would like to see it ...

Updated

12 years ago
Flags: testcase+
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+
Attachment #195591 - Flags: approval1.7.13?
Attachment #195591 - Flags: approval-aviary1.0.8?
(Assignee)

Comment 10

12 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+
(Assignee)

Updated

12 years ago
Blocks: 260663
Removing blocking flags from the old branches
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8+
Group: security

Updated

11 years ago
Flags: in-testsuite+ → in-testsuite?
You need to log in before you can comment on or make changes to this bug.