Closed
Bug 84031
Opened 23 years ago
Closed 23 years ago
Fix nsIFrame::GetBidiProperty problem
Categories
(Core :: Layout: Text and Fonts, defect, P3)
Core
Layout: Text and Fonts
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)
835 bytes,
patch
|
Details | Diff | Splinter Review | |
1018 bytes,
patch
|
Details | Diff | Splinter Review | |
1.82 KB,
patch
|
Details | Diff | Splinter Review | |
452 bytes,
patch
|
Details | Diff | Splinter Review | |
654 bytes,
patch
|
Details | Diff | Splinter Review | |
1.16 KB,
patch
|
Details | Diff | Splinter Review | |
6.17 KB,
patch
|
Details | Diff | Splinter Review | |
22.78 KB,
patch
|
Details | Diff | Splinter Review | |
4.22 KB,
patch
|
Details | Diff | Splinter Review |
Assignee | ||
Comment 1•23 years ago
|
||
This is a potential crashing issue.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.2
Assignee | ||
Comment 2•23 years ago
|
||
Assignee | ||
Comment 3•23 years ago
|
||
Assignee | ||
Comment 4•23 years ago
|
||
Assignee | ||
Comment 5•23 years ago
|
||
find the origional problem from 83448
Comment 6•23 years ago
|
||
You beat me to filing this :-) Attaching a few more diffs
Comment 7•23 years ago
|
||
Comment 8•23 years ago
|
||
Comment 9•23 years ago
|
||
Comment 10•23 years ago
|
||
Assignee | ||
Comment 11•23 years ago
|
||
simon- is that all ?
Comment 12•23 years ago
|
||
Yes, that covers all the cases.
Assignee | ||
Comment 13•23 years ago
|
||
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
Comment 14•23 years ago
|
||
Switching QA contact to giladehven@hotmail.com.
Keywords: intl
QA Contact: andreasb → giladehven
Assignee | ||
Comment 15•23 years ago
|
||
pdt+ base on 6/11 pdt meeting.
Assignee | ||
Updated•23 years ago
|
Whiteboard: [PDT+]
Assignee | ||
Updated•23 years ago
|
Whiteboard: [PDT+] → [PDT+]patch ready
Comment 16•23 years ago
|
||
sr=attinasi
Assignee | ||
Updated•23 years ago
|
Whiteboard: [PDT+]patch ready → [PDT+]sr=attinasi wait for a=
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.
Assignee | ||
Comment 19•23 years ago
|
||
>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.
Assignee | ||
Comment 20•23 years ago
|
||
we probably won't crash except on those machine which sizeof(void*) != sizeof(long) cc jdunn
Comment 21•23 years ago
|
||
r=simon@softel.co.il for Frank's patches
Comment 22•23 years ago
|
||
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
Comment 23•23 years ago
|
||
Changing component to "Bidi"
Component: Internationalization → BiDi Hebrew & Arabic
Assignee | ||
Comment 24•23 years ago
|
||
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+]
Assignee | ||
Comment 25•23 years ago
|
||
Assignee | ||
Comment 26•23 years ago
|
||
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.
Comment 27•23 years ago
|
||
>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
Assignee | ||
Updated•23 years ago
|
Whiteboard: [PDT+]patch ready, need review. → [PDT+]r=simon@softel.co.il
Assignee | ||
Updated•23 years ago
|
Whiteboard: [PDT+]r=simon@softel.co.il → [PDT+]r=simon@softel.co.il ask kin to sr 11:23 6/20
Comment 28•23 years ago
|
||
I agree with simon, lets take care of nsCaret.cpp so we can put this entire issue to bed. sr=kin@netscape.com
Assignee | ||
Comment 29•23 years ago
|
||
Assignee | ||
Comment 30•23 years ago
|
||
kin can you r/sr the additional changes ?
Comment 31•23 years ago
|
||
r/sr=kin@netscape.com on the nsCaret.cpp changes. (06/20/01 14:07 attatchment)
Comment 32•23 years ago
|
||
r=nhotta for 06/20/01 14:07 additional change in nsCaret.cpp.
Assignee | ||
Updated•23 years ago
|
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
Comment 33•23 years ago
|
||
a=blizzard on behalf of drivers for 0.9.2
Updated•23 years ago
|
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
Assignee | ||
Comment 34•23 years ago
|
||
fixed and closed
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 35•23 years ago
|
||
simon@softel.co.il or giladehven - can you pls verify this bug?
Comment 36•23 years ago
|
||
QA contact to Zach, who is the default QA now.
QA Contact: giladehven → zach
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.
Description
•