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)
Tracking
()
VERIFIED
FIXED
People
(Reporter: peterlubczynski-bugs, Assigned: peterlubczynski-bugs)
References
Details
(Keywords: crash, topcrash, Whiteboard: [nsbeta3+] [PDTP1])
Attachments
(2 files)
1.97 KB,
text/html
|
Details | |
595 bytes,
patch
|
Details | Diff | Splinter Review |
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]
Assignee | ||
Comment 1•24 years ago
|
||
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
Comment 3•24 years ago
|
||
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 ?
Assignee | ||
Comment 4•24 years ago
|
||
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
Assignee | ||
Comment 5•24 years ago
|
||
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
Assignee | ||
Comment 7•24 years ago
|
||
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. :-)
Comment 12•24 years ago
|
||
The patch from bug 52917 looks great. I will run it through the block and table regression tests, as well as the top100.
Comment 13•24 years ago
|
||
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.)
Comment 14•24 years ago
|
||
adding self to cc list - i have a reproducible crash that looks similar and I want to make sure it's fixed.
Comment 15•24 years ago
|
||
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?
Comment 16•24 years ago
|
||
Steve, I'm going to look into it now...
Comment 17•24 years ago
|
||
*** Bug 53383 has been marked as a duplicate of this bug. ***
Comment 18•24 years ago
|
||
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+?
Comment 19•24 years ago
|
||
*** Bug 53395 has been marked as a duplicate of this bug. ***
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...
Assignee | ||
Comment 23•24 years ago
|
||
..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
Comment 24•24 years ago
|
||
Agreeing that topcrash, that is as understandable and easy to fix as this, should be a P1. Putting in PDTP1
Whiteboard: nsbeta3+ → [nsbeta3+] [PDTP1]
Assignee | ||
Comment 25•24 years ago
|
||
Sorry guys, the tree is STILL closed....
Assignee | ||
Comment 26•24 years ago
|
||
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?
Comment 28•24 years ago
|
||
*** Bug 53043 has been marked as a duplicate of this bug. ***
Comment 29•24 years ago
|
||
QA: please verify the test case in bug 53043 as well.
Comment 30•24 years ago
|
||
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 → ---
Assignee | ||
Comment 31•24 years ago
|
||
looks like I just missed the branching.... I don't have that branch on my machine yet, so Bienvenu, can you check it in?
Comment 32•24 years ago
|
||
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.
Comment 33•24 years ago
|
||
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.
Comment 34•24 years ago
|
||
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 ago → 24 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•