152056 - Uninitialized memory read in nsTextFrame::GetPosition

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[[e-mail] (gone - use [e-mail] instead)]

I see a UMR in nsTextFrame::GetPosition when loading http://www.mozilla.org on 
the latest trunk under Purify.

Here's the full stack.

[W] UMR: Uninitialized memory read in nsTextFrame::GetPosition(nsIPresContext 
*,nsPoint const&,nsIContent * *,int&,int&) {2 occurrences}
    Reading 2 bytes from 0x0013f562 (2 bytes at 0x0013f562 uninitialized)
    Address 0x0013f562 points into a thread's stack 
    Address 0x0013f562 is 66 bytes past the start of local 
variable 'paintBuffer' in nsTextFrame::GetPosition(nsIPresContext *,nsPoint 
const&,nsIContent * *,int&,int&)
    Thread ID: 0x5e4
    Error location
    nsTextFrame::GetPosition(nsIPresContext *,nsPoint const&,nsIContent * 
*,int&,int&) [nsTextFrame.cpp:3496]
            PRInt32 i;
            for (i = 0;i <= mContentLength; i ++){
              if ((ip[i] >= aContentOffset) && //reverse mapping
 =>               (! IS_LOW_SURROGATE(paintBuffer.mBuffer[ip[i]-
mContentOffset]))) {
                  aContentOffset = i + mContentOffset;
                  break;
              }
    nsTextFrame::GetContentAndOffsetsFromPoint(nsIPresContext *,nsPoint 
const&,nsIContent * *,int&,int&,int&) [nsTextFrame.cpp:3546]
        newPoint.x = 0;
          else
            newPoint.x = aPoint.x;
     =>   nsresult rv = GetPosition(aCX, newPoint, aNewContent, aContentOffset, 
aContentOffsetEnd);
          if (NS_FAILED(rv))
            return rv;
          if (aContentOffset == mContentOffset)
    nsFrame::GetNextPrevLineFromeBlockFrame(nsIPresContext *,nsPeekOffsetStruct 
*,nsIFrame *,int,signed char) [nsFrame.cpp:3119]
                                                  getter_AddRefs(aPos-
>mResultContent),
                                                  aPos->mContentOffset,
                                                  aPos->mContentOffsetEnd,
     =>                                           aPos->mPreferLeft);
                }
                if (NS_SUCCEEDED(result))
                {
    nsBlockFrame::HandleEvent(nsIPresContext *,nsGUIEvent *,nsEventStatus *) 
[nsBlockFrame.cpp:5879]
    PresShell::HandleEventInternal(nsEvent *,nsIView *,UINT,nsEventStatus *) 
[nsPresShell.cpp:6181]
    PresShell::HandleEvent(nsIView *,nsGUIEvent *,nsEventStatus *,int,int&) 
[nsPresShell.cpp:6089]
    nsViewManager::HandleEvent(nsView *,nsGUIEvent *,int) 
[nsViewManager.cpp:2083]
    nsView::HandleEvent(nsViewManager *,nsGUIEvent *,int) [nsView.cpp:304]
    nsViewManager::DispatchEvent(nsGUIEvent *,nsEventStatus *) 
[nsViewManager.cpp:1890]
    HandleEvent    [nsView.cpp:80]

Comment 1

[[e-mail] (gone - use [e-mail] instead)]

1.360 <[e-mail]> 26 Mar 2002 19:28
fix bug 130441 and 122584
support surrogate in text-align: jutify , selection and cusor movement
r=shanjian/smontagu sr=kin a=asa

I think this should go to Frank Tang - as a result of the above.

Comment 2

[[e-mail] (gone - use [e-mail] instead)]

for clarification, this leak actually comes from the urlbar dropping down.

Comment 3

[[e-mail] (gone - use [e-mail] instead)]

UMR's can be particularly nasty, so I'm nominating this.

Looks like in this case, you need to declare/init the local variable paintBuffer.

Comment 4

[Frank Tang]

ok, I will work on it. Stephen: is [adt2] a correct rating of this bug?

Comment 5

[[e-mail] (gone - use [e-mail] instead)]

Beats me, I'm QA, not ADT.

Comment 6

[Frank Tang]

Stephen: I understand you are not ADT. Just try to got your personal feeling how
important this bug is. 

shanjian- I am swamp on other stuff. Is that possible you can look at the code
around there for me?

Comment 7

[Frank Tang]

reassign to shanjian

Comment 8

[Shanjian Li]

Attachment: patch

Comment 9

[Shanjian Li]

In PrepareUnicodeText(), ip[mContentLength] is set to go beyond the border of
text. That should explain the problem. 

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 88665 [details] [diff] [review]
patch

>         aContentOffset = indx + mContentOffset;

Looks like indx is used uninitialized in most parts of this routine,
and then under certain circumstances it's incremented by one, but
otherwise untouched. That can't be right, is it? Is BinarySearchForPosition
supposed to be taking &indx? Looks like it, and &textWidth, too.

>-        for (i = 0;i <= mContentLength; i ++){
>+        for (i = 0; i < mContentLength; i++){

This I get

>           if ((ip[i] >= aContentOffset) && //reverse mapping
>               (! IS_LOW_SURROGATE(paintBuffer.mBuffer[ip[i]-mContentOffset]))) {
>-              aContentOffset = i + mContentOffset;
>               break;
>           }
>         }
>+        aContentOffset = i + mContentOffset;

but this changes the meaning a lot. Could you explain this? Before if
the ip[i] condition wasn't met and you eventually ran out the loop
then aContentOffset was left as indx + mContentOffset. Notwithstanding
my worries about indx in the first place, now in the same case you'll
set aContentOffset to mContentLength + mContentOffset.

Comment 11

[Daniel Veditz [:dveditz]]

CC'ing mjudge for a look since he wrote this section originally and can maybe
tell us if changing assumptions are leading to my confusion.

Comment 12

[Shanjian Li]

I didn't check indx.
The code around there looks pretty complicated, so I just focus on the problem
area and try to fix the problem without touching anything else. Obviously (at
least to me), when i == mContentLength, paintBuffer.mBuffer[ip[mContentlength] -
mContentOffset] is accessed. If you take a look at implementation of
"PrepareUnicodeText", you will see that ip[mContentLength] is set beyond the
border of content. I guess that's why purify complains. 

So my approach to the problem is try to handle this (i==mContentLength)
gracefully. If ip[i] is always less than aContentOffset, set 
"aContentOffset = i + mContentOffset", ("i" will be mContentOffset) 
should be fine since aContentOffset should never excfeeds
mContentLength+mContentOffset. Other than this situation, my code works exactly
the same as existing code. 

Comment 13

[Daniel Veditz [:dveditz]]

unless indx == mContentLength your code *does* differ in this case, and since
that case doesn't appear to be involved with the UMR I'm trying to understand
why that change and what it is doing. The obvious bogosity of indx in the
original code isn't helping my understanding.

What sort of test case would exercise this code so that we can see the effects
of hitting or not hitting the break statement and what difference it makes?

Comment 14

[[e-mail] (gone - use [e-mail] instead)]

Dan, I saw this when autocompleting in the URL bar - the url was, of course,
http://www.mozilla.org, for which I already had an existing entry.

Comment 15

[mjudge]

indx is passed into BinarySearchForPosition. it is then set to the value based
on 0 from the start of the frame.  So there should be no worries about indx
since it must be initialized in BinarySearchForPosition.  We can set indx to =0
if you think that would be more correct, but its really not necessary in this
case. Let me look at this patch now.

Comment 16

[mjudge]

The changing of a <= to a < seems correct.  The moving of the aContentOffset
assignment outside the loop seems correct as well because in the unlikely event
of the loop dropping out by searching the actually length of content and not
finding the proper rendered string it will set the offset to be at the end of
the content.  

so in summation, 

*indx looks initialized in all paths to me (set it to 0 on creation if we want)
*moving aContentOffset outside the loop seems correct as well as to let the
failure put the position at the end of the text length. (i am not sure when this
is a good thing. I may put an assert here for my own build to see when that happens)

MJ

Comment 17

[Frank Tang]

so... mj, can you give a r= for this one ?

Comment 18

[mjudge]

sure I will give a r= sorry didnt know you wanted me to do that.  Its labeled as 
needs work by someone so I am assuming whomever wants more work will speak up 
before this gets checked in?

Comment 19

[mjudge]

Comment on attachment 88665 [details] [diff] [review]
patch

marking reviewed.

Comment 20

[Shanjian Li]

chris, could you sr?

Comment 21

[[e-mail] (gone - use [e-mail] instead)]

Well, this patch quietens Purify down, but don't consider my authoritative by
any means...

Comment 22

[Shanjian Li]

Comment on attachment 88665 [details] [diff] [review]
patch

remove needs-work. Please add your comment if you don't agree.

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 88665 [details] [diff] [review]
patch

sr=dveditz

Comment 24

[Shanjian Li]

fix checked into trunk.

Comment 25

[[e-mail] (gone - use [e-mail] instead)]

Verified fixed, thanks for doing this.

Comment 26

[Jaime Rodriguez, Jr.]

adt1.0.1- on behalf of the ADT. should disagree with the minus and want this
checked into the 1.0.1 release, pls renominate by removing the minus fron
adt1.0.1-, leaving adt1.0.1 in the keywords. 

let's get this in the next release. thanks!