352514 - Possibly unsafe assignment in nsBidiKeyboard::IsLangRTL(PRBool *aIsRTL)

Status: RESOLVED WONTFIX

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

Description

[Maxim Radugin]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b2) Gecko/20060912 BonEcho/2.0b2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b2) Gecko/20060912 BonEcho/2.0b2

in /cvsroot/mozilla/widget/src/windows/nsBidiKeyboard.cpp:

NS_IMETHODIMP nsBidiKeyboard::IsLangRTL(PRBool *aIsRTL)
{
*aIsRTL = PR_FALSE;
...

Since aIsRTL is a pointer to a variable, it should be checked against NULL before making any assignments to it.

Reproducible: Always

Comment 1

[Simon Montagu :smontagu]

The same goes for all the other implementations of nsBidiKeyboard too

Comment 2

[Ryan Jones-Ward [:sciguyryan]]

http://lxr.mozilla.org/mozilla/source/widget/src/windows/nsBidiKeyboard.cpp#86

For those who may like a lxr linkup.

-- Ryan

Comment 3

[Karthik Sarma]

Attachment: Returns null pointer error if necessary before dereference

Can do the others the same way if this is good

Comment 4

[Simon Montagu :smontagu]

Comment on attachment 238543 [details] [diff] [review]
Returns null pointer error if necessary before dereference

This is the right fix, but please follow local style: insert a space after the "if" and put the return on a separate line.

Please make that change, apply it to all the versions, attach a new patch and rerequest review.

Comment 5

[Karthik Sarma]

(In reply to comment #4)
> (From update of attachment 238543 [details] [diff] [review] [edit])
> This is the right fix, but please follow local style: insert a space after the
> "if" and put the return on a separate line.
> 
> Please make that change, apply it to all the versions, attach a new patch and
> rerequest review.
> 

Will do, thanks

Comment 6

[Karthik Sarma]

Attachment: Patches all the nsBidiKeyboard.cpp files in widget/src to prevent dereferencing null pointer

Comment 7

[Maxim Radugin]

(In reply to comment #6)
> Created an attachment (id=238684) [edit]
> Patches all the nsBidiKeyboard.cpp files in widget/src to prevent dereferencing
> null pointer
> 

What about space between "if" and opening bracket?

Comment 8

[Karthik Sarma]

Attachment: Patches all the nsBidiKeyboard.cpp files in widget/src to prevent dereferencing null pointer with correct style

Sorry for the earlier mistakes... that is what I get for copy pasting

Comment 9

[Simon Montagu :smontagu]

Comment on attachment 238693 [details] [diff] [review]
Patches all the nsBidiKeyboard.cpp files in widget/src to prevent dereferencing null pointer with correct style

Sorry, I meant to tell you in my last comment that you should specify who you are requesting review from. Otherwise the request may not get noticed.

r=me.

Comment 10

[Karthik Sarma]

(In reply to comment #9)
> (From update of attachment 238693 [details] [diff] [review] [edit])
> Sorry, I meant to tell you in my last comment that you should specify who you
> are requesting review from. Otherwise the request may not get noticed.
> 
> r=me.
> 
Alright, thanks! Am I supposed to request super-review for this now?

Comment 11

[Simon Montagu :smontagu]

(In reply to comment #10)
> Alright, thanks! Am I supposed to request super-review for this now?
> 

Yes. See http://www.mozilla.org/hacking/reviewers.html, but I think the super-reviewers listed there for widget/ aren't active. Try [e-mail], who is module owner of widget/, or one of the "catch-all" super-reviewers.

Comment 12

[Karthik Sarma]

(In reply to comment #11)
> (In reply to comment #10)
> > Alright, thanks! Am I supposed to request super-review for this now?
> > 
> 
> Yes. See http://www.mozilla.org/hacking/reviewers.html, but I think the
> super-reviewers listed there for widget/ aren't active. Try [e-mail],
> who is module owner of widget/, or one of the "catch-all" super-reviewers.
> 

Will do, thanks

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 238693 [details] [diff] [review]
Patches all the nsBidiKeyboard.cpp files in widget/src to prevent dereferencing null pointer with correct style

Sorry, but I don't think we should check these for null. Yes, technically this is a public IDL interface and someone could script it and pass null and cause a crash from Javascript, which shouldn't happen. If that's the concern here then we should simply make nsIBidiKeyboard a C++-only interface in a .h file ... and then we would make IsLangRTL() just return a PRBool and there'd be no more null issue.

But as is, I don't think this is worth doing. We know we're not going to pass in null.

Comment 14

[Karthik Sarma]

We should close bug then?

Comment 15

[Simon Montagu :smontagu]

Yeah