Closed Bug 52722 Opened 24 years ago Closed 24 years ago

ABW: Writing past array bounds in nsTextTransformer::ScanNormalUnicodeText_F causes crash

Categories

(Core :: Layout, defect, P1)

x86
Windows NT
defect

Tracking

()

VERIFIED FIXED

People

(Reporter: peterlubczynski-bugs, Assigned: peterlubczynski-bugs)

References

Details

(Keywords: crash, topcrash, Whiteboard: [nsbeta3+] [PDTP1])

Attachments

(2 files)

Load the test case. Notice the crash. 

According to Prurify, we are going past the array bounds:

[E] ABW: Array bounds write in 
nsTextTransformer::ScanNormalUnicodeText_F(int,int *,int *) {11 occurrences}
        Writing 2 bytes to 0x09049fd6 (2 bytes at 0x09049fd6 illegal)
        Address 0x09049fd6 is 1 byte past the end of a 262 byte block at 
0x09049ed0
        Address 0x09049fd6 points to a C++ new block 
        Thread ID: 0x4d
        Error location
            nsTextTransformer::ScanNormalUnicodeText_F(int,int *,int *) 
[nsTextTransformer.cpp:435]
                      firstChar = ' ';
                      *aWasTransformed = PR_TRUE;
                    }
             =>     mTransformBuf.mBuffer[mBufferPos++] = firstChar;
                
                    if (!breakBetween) {
                      // Find next position
            nsTextTransformer::GetNextWord(int,int *,int *,int *,int *,int,int) 
[nsTextTransformer.cpp:747]
            nsTextFrame::MeasureText(nsIPresContext *,nsHTMLReflowState 
const&,nsTextTransformer&,nsILineBreaker 
*,TextStyle::nsTextFrame&,TextReflowData::nsTextFrame&) [nsTextFrame.cpp:3646]
            nsTextFrame::Reflow(nsIPresContext 
*,nsHTMLReflowMetrics&,nsHTMLReflowState const&,UINT&) [nsTextFrame.cpp:4247]
            nsLineLayout::ReflowFrame(nsIFrame *,nsIFrame * 
*,UINT&,nsHTMLReflowMetrics *,int&) [nsLineLayout.cpp:919]
            
nsBlockFrame::ReflowInlineFrame(nsBlockReflowState&,nsLineLayout&,nsLineBox 
*,nsIFrame *,BYTE *) [nsBlockFrame.cpp:4343]
            
nsBlockFrame::DoReflowInlineFrames(nsBlockReflowState&,nsLineLayout&,nsLineBox 
*,int *,BYTE *,int,int) [nsBlockFrame.cpp:4227]
            nsBlockFrame::DoReflowInlineFramesAuto(nsBlockReflowState&,nsLineBox 
*,int *,BYTE *,int,int) [nsBlockFrame.cpp:4161]
            nsBlockFrame::ReflowInlineFrames(nsBlockReflowState&,nsLineBox *,int 
*,int,int) [nsBlockFrame.cpp:4106]
            nsBlockFrame::ReflowLine(nsBlockReflowState&,nsLineBox *,int *,int) 
[nsBlockFrame.cpp:3243]
Summary: Writing past array bounds in nsTextTransformer::ScanNormalUnicodeText_F causes crash → ABW: Writing past array bounds in nsTextTransformer::ScanNormalUnicodeText_F causes crash
Erik or Frank:  I'm hoping this is something one of you could look at, should be 
an easy fix.  It was uncovered when Peter checked in some performance work on 
this class (reducing the default buffer size), but I'm pretty sure that's not 
the cause of the bug.  I think this bug was lurking in the code all along, 
and could easily occur on other pages if we were to just revert the buffer size 
(which is a pretty severe bloat hit!)  It's worth investigating in it's current 
state.

Peter, if you have free time, feel free to dig deeper into this one and see if 
you can find the code that's doing the buffer overrun.
Assignee: clayton → erik
according to cvsblame, buster is the last one who touch that line of code. I 
think you need to call 
mTransformBuf.GrowTo(mBufferPos+1);
before write to mTransformBuf.mBuffer[mBufferPos++]

peter- can you verify that fix the problem ?
Increasing the buffer by 1 does appear to fix this problem. However, I'm not 
quite sure why it works. 

adding crash keyword
Keywords: crash
Oh...I get it now. duh! This was an easy fix.
Peter:  please attach the patch.  Assigning to Peter, since he got there first. 
Thanks!

Recommending nsbeta3+, P1.  Crasher on high profile CSS test suite.  Easy, low 
risk fix.  Will do formal review when patch is attached.
Assignee: erik → peterlubczynski
Keywords: nsbeta3
Attached patch trivial patchSplinter Review
This is showing up in talkback data (see n.p.m.crash-data or
http://www.mozilla.org/projects/seamonkey/reports/n6analysis.html ) on 
builds since 9-12.  Could this patch get checked in soon?

I saw a duplicate of this bug somewhere else that also had a patch 
attached.
Keywords: topcrash
*** Bug 52917 has been marked as a duplicate of this bug. ***
The patch attached to the other bug seems a little more thorough.  Please 
decide which patch is better and try to get it checked in soon. :-)
I'm testing the patch now.
Priority: P3 → P1
The patch from bug 52917 looks great.  I will run it through the block and table 
regression tests, as well as the top100.
I've run through the top 100 sites, plus all of the block and table regression
tests.  Everything looks good.  However, since this code involves unicode
transformation, it would be good if erik or one of the other I18N developers ran
it through whatever test suites they have as well.
r=buster, pending ok from erik's group (for the patch attached to bug 52917,
which is a bit more complete than the one attached to this bug.)
adding self to cc list - i have a reproducible crash that looks similar and I
want to make sure it's fixed.
come on, PDT, this needs an nsbeta3+

Erik: as mentioned before, the patch could use additional testing on I18N sites.
 Please help in this regard.  Is there a set of URL's we could test against?
Some standard test suite you use for regression testing?
Steve, I'm going to look into it now...
*** Bug 53383 has been marked as a duplicate of this bug. ***
I crash in here several times a day (with unique web pages and mail messages).
It's also big on the top talkback crashers. Why isn't it nsbeta3+?
*** Bug 53395 has been marked as a duplicate of this bug. ***
Marking nsbeta3+.
Whiteboard: nsbeta3+
patch checked-in
Status: NEW → ASSIGNED
Looks like patch was backed out.  I'm not sure why, since I think all it needed
was not re-declaring rv the second time (i.e., change second "nsresult rv" to
"rv").  Spaces instead of tabs would also be nice...
..no longer a tree bustage virgin....

I found the problem. The patch is fine just when I applied it to my tree, the 
second hunk was placed in the wrong spot. I'll try this again when the tree goes 
green
Agreeing that topcrash, that is as understandable and easy to fix as this, 
should be a P1.  Putting in PDTP1
Whiteboard: nsbeta3+ → [nsbeta3+] [PDTP1]
Sorry guys, the tree is STILL closed....
patch checked-in, marking as FIXED
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
It looks like the patch was checked in to the trunk, but not on the branch.  (It
looks like after the branch point.)  Shouldn't this be checked in to the branch too?
*** Bug 53043 has been marked as a duplicate of this bug. ***
QA: please verify the test case in bug 53043 as well.
Reopening - this fix is definitely not on the branch. I'm happy to check it in
onto the branch, if no one else wants to. I'd need to go see what patch was
checked in, however.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
looks like I just missed the branching....

I don't have that branch on my machine yet, so Bienvenu, can you check it in?
sure, I'd be happy too. I'll check if the tree is open, and if so, check it in.
I've built and run with the patch on the branch.
I can't check this in because the branch is closed to netscape people because of
some problem with the mac commercial build - I'd love to check it in to get it
out of the talkback logs ASAP, but I can't. If you get a branch tree pulled
before I'm allowed to check in, could you just check in?  Or if you can find
some other mozilla person to check in, we could do it that way...if I'm ever
allowed to check in before I die, I'll annotate the bug and re-close it.
OK, fix checked into branch. it looks like the commercial tree opened judging by
the number of netscape people checking into both trees, though there was no
announcement...
Status: REOPENED → RESOLVED
Closed: 24 years ago24 years ago
Resolution: --- → FIXED
Fixed in the Sept 29th build.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: