Closed Bug 84031 Opened 23 years ago Closed 23 years ago

Fix nsIFrame::GetBidiProperty problem

Categories

(Core :: Layout: Text and Fonts, defect, P3)

defect

Tracking

()

VERIFIED FIXED
mozilla0.9.2

People

(Reporter: ftang, Assigned: ftang)

References

Details

(Keywords: intl, Whiteboard: [PDT+]r=simon@softel.co.il/nhotta sr=kin ask for a= 2:18 6/20, critical for 0.9.2)

Attachments

(9 files)

 
This is a potential crashing issue.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.2
find the origional problem from 83448
You beat me to filing this :-) Attaching a few more diffs
simon- is that all ?
Yes, that covers all the cases.
Don't know the real user impact yet. Hard to justify that we need it for 
moz0.9.2. But from an engineer's instinct. We will see some problem without 
fixing it. .... Mark it as a P2.
Priority: -- → P3
Switching QA contact to giladehven@hotmail.com.
Keywords: intl
QA Contact: andreasb → giladehven
pdt+ base on 6/11 pdt meeting.
Whiteboard: [PDT+]
Whiteboard: [PDT+] → [PDT+]patch ready
sr=attinasi
Whiteboard: [PDT+]patch ready → [PDT+]sr=attinasi wait for a=
Blocks: 83989
I don't see a review (just super-review) noted on the bug yet.  Also, the
inconsistency between using sizeof(type) vs. sizeof(variable) is somewhat
bothersome -- sizeof(variable) seems safer in case you change the type of the
variable although somewhat dangerous in case you already have the variable
dereferenced (e.g., in a getter that takes a PRUint8*).

A few other notes looking at related code (although you don't need to fix these
now):
 * The implementations of [Get/Set]BidiProperty in nsFrame.cpp should be
declared NS_IMETHODIMP rather than nsresult.
 * All 3 declarations of SetBidiProperty should probably not be |const|.
Also, it's much easier if you attach diffs in a single patch file rather than
one per file.
>I don't see a review (just super-review) noted on the bug yet.
r= for simon's changes.
simon, can you r= for my patches?
>sizeof(variable) seems safer
Ok, I will change all the patch to use that.

>A few other notes looking at related code (although you don't need to fix these
now):
> * The implementations of [Get/Set]BidiProperty in nsFrame.cpp should be
>   declared NS_IMETHODIMP rather than nsresult.
> * All 3 declarations of SetBidiProperty should probably not be |const|.
ok, file bug 86586 about that issue. Let's focus on what we intent to fix here. 

>Also, it's much easier if you attach diffs in a single patch file rather than
one per file.

But we found them one by one. And it is also mixed with all my other fixes.
There are no easy for me to generate a single diff files. I have to manully
remove some diff in some files since in include some other fixes. 

we probably won't crash except on those machine which 
sizeof(void*) != sizeof(long)

cc jdunn 
r=simon@softel.co.il for Frank's patches
I am adding cls.

Frank, I couldn't see the instance where the issue of 
sizeof(void*) .vs. sizeof(long) comes in, can you point it out?
But if this is the case, then it should be corrected.

However I do have these comments:

in nsIFrame.h the 'size' parm is given a type of PRInt32, why?
What is the default type returned by sizeof, why isn't this used.
And if PRInt32 is used as the parm, then shouldn't the call to
it be cast to it?  (i.e  (PRInt32)sizeof(nsIFrame*)) To remove
ambiguities and warnings?

I see 2 different invocations of this call.
What I consider the correct way (sizeof the variable)

    PRInt32 alignRight;
    frame->GetBidiProperty(aPresContext, nsLayoutAtoms::baseLevel,
                           (void**) &alignRight, sizeof(alignRight));

versus what I would call the wrong way, sizeof the type (types have
a tendency to change).  
   nsIFrame* nextBidi;
   aChild->GetBidiProperty(aPresContext, nsLayoutAtoms::nextBidi,
                           (void**) &nextBidi, sizeof(nsIFrame*));

Actually I don't care WHICH is used, but only do it one way.  
However, the way that reduces the chances of programmer error is 
sizeof(variable) 

AND yes, when you resubmit the patch, put all the diffs in ONE patch

 
Changing component to "Bidi"
Component: Internationalization → BiDi Hebrew & Arabic
I will attach a patch again.
My feeling is that we don't need to fix this except for the platform which 
sizeof(long) != sizeof(void*) 
If sizeof(long) != sizeof(void*) , then we will crash since the default aSize is
set to sizeof(void*) but there are some cases we don't pass in sizeof(long) with 
the variable of long. in that case, we will corrupt the stack.

Whiteboard: [PDT+]sr=attinasi wait for a= → [PDT+]
ok, the patch looks big and scary but it is very very simple. Let me explain why 
this bug is critical and what is in the patch here:
The root of the problem is a badly design XPCOM interface method
nsIFrame::GetBidiProperty(nsIPresContext* aPresContext,
                              nsIAtom*        aPropertyName,
                              void**          aPropertyValue,
                              long            aSize = sizeof(void*) )
basicall, this interface pass in an aPresContext and a selector- aPropertyName 
to decide what bidi property want to get, and then pass in void** as the address 
of the output buffer and a long aSize as the length of the output. The very very 
bad part is it has a default value sizeof(void*) for aSize. 

First of all, this method patten is not a good xpcom method since we use void** 
here for output paramenter. We should fix this in the long term. However, that 
itself won't make this bug potential crasher. The problem is the default value 
of aSize and also in some cases, we see the caller code like below:

long avalue;
frame->GetBidiProperties(aPresContext, somebidiatom, (void**)&avalue);
notice it does not pass in the 4th argument. Therefore the default value 
sizeof(void*) will be used. 
On window, this probably won't cause any crash, however, on any platform which 
sizeof(void*) is bigger than sizeof(long), the GetBidiProperties will copy 
larger amount of memory than the size of avalue and damage the stack.
on any platform which sizeof(void*) is smaller than sizeof(long), the out value 
of long will be a bad value. 

This bug is filed because we hit such crash in editor (see bug 83448). We fix 
that bug and decide we need to do the minimum clean up that prevent more crasher 
like this.

So... the patch contains the following changes
1. Change GetBidiProperty method:
1.1. change the type of the 4th argument from long to size_t (suggested by 
jdunn, use the same type sizeof return)
1.2. remove the default value of the 4th argument - This will make sure there 
are no place use default value so we won't miss any place. If we miss any 
caller, we will break the build process. 
2. Change the SetBidiProperty method- I don't think this is essential. Just 
react to david baron's review comment- remove const 
3. If the caller does not pass in the 4th argument, pass in sizeof(variable) as 
the 4th argument.
4. If the caller pass sizeof(datatype) as the 4th argument, pass 
sizeof(variable) as the the 4th arguemnt. This is reflect to david and jdun's 
review comment. I don't think this part is critical, so I only change the files 
which we are changing.  
5. if the caller use 'long' as data type, change it to 'PRInt32' or 'PRUint8' 
depend on what property we copy.

The really critical changes is '3'.

Whiteboard: [PDT+] → [PDT+]patch ready, need review.
>4. If the caller pass sizeof(datatype) as the 4th argument, pass 
>sizeof(variable) as the the 4th arguemnt. This is reflect to david and jdun's 
>review comment. I don't think this part is critical, so I only change the files 
>which we are changing.  

Agreed that it's not critical, but there is only one other file which calls
|GetBidiProperty|, mozilla\layout\base\src\nsCaret.cpp, so why not change that
one too so that we are completely consistent?

Otherwise r=simon@softel.co.il
Whiteboard: [PDT+]patch ready, need review. → [PDT+]r=simon@softel.co.il
Whiteboard: [PDT+]r=simon@softel.co.il → [PDT+]r=simon@softel.co.il ask kin to sr 11:23 6/20
I agree with simon, lets take care of nsCaret.cpp so we can put this entire 
issue to bed.

sr=kin@netscape.com
kin can you r/sr the additional changes ?
r/sr=kin@netscape.com on the nsCaret.cpp changes. (06/20/01 14:07 attatchment)
r=nhotta for 06/20/01 14:07 additional change in nsCaret.cpp.
Whiteboard: [PDT+]r=simon@softel.co.il ask kin to sr 11:23 6/20 → [PDT+]r=simon@softel.co.il/nhotta sr=kin ask for a= 2:18 6/20
a=blizzard on behalf of drivers for 0.9.2
Whiteboard: [PDT+]r=simon@softel.co.il/nhotta sr=kin ask for a= 2:18 6/20 → [PDT+]r=simon@softel.co.il/nhotta sr=kin ask for a= 2:18 6/20, critical for 0.9.2
fixed and closed
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
simon@softel.co.il or giladehven - can you pls verify this bug?
QA contact to Zach, who is the default QA now.
QA Contact: giladehven → zach
VERIFIED FIXED
Status: RESOLVED → VERIFIED
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: zach → layout.fonts-and-text
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: