This is a potential crashing issue.

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.

Switching QA contact to giladehven@hotmail.com.

pdt+ base on 6/11 pdt meeting.

sr=attinasi

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"

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.

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'.

>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

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.

a=blizzard on behalf of drivers for 0.9.2

fixed and closed

simon@softel.co.il or giladehven - can you pls verify this bug?

QA contact to Zach, who is the default QA now.

VERIFIED FIXED