1276901 - [Static Analysis][Dereference after null check] In function nsMenuBarListener::KeyPress

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Andi [:andi]]

The Static Analysis tool Coverity added that variable |nativeKeyEvent| can cause a null pointer dereference in the following context:

1. null checked: 

>>    bool hasAccessKeyCandidates = charCode != 0;
>>    if (!hasAccessKeyCandidates) {
>>      if (nativeKeyEvent) {
>>        AutoTArray<uint32_t, 10> keys;
>>        nativeKeyEvent->GetAccessKeyCandidates(keys);
>>        hasAccessKeyCandidates = !keys.IsEmpty();
>>      }
>>    }

2. dereference without null check

>>    else if (nativeKeyEvent->mMessage == eKeyPress && keyCode == NS_VK_F10) {

Comment 1

[Andi [:andi]]

Attachment: Bug 1276901 - prevent null pointer dereference on |nativeKeyEvent|.

Review commit: https://reviewboard.mozilla.org/r/56542/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56542/

Comment 2

[Andi [:andi]]

Comment on attachment 8758220 [details]
Bug 1276901 - prevent null pointer dereference on |nativeKeyEvent|.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56542/diff/1-2/

Comment 3

[Andi [:andi]]

Comment on attachment 8758220 [details]
Bug 1276901 - prevent null pointer dereference on |nativeKeyEvent|.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56542/diff/2-3/

Comment 4

[Neil Deakin]

https://reviewboard.mozilla.org/r/56542/#review53094

::: layout/xul/nsMenuBarListener.cpp:214
(Diff revision 1)
>    {
>      // If accesskey handling was forwarded to a child process, wait for
>      // the mozaccesskeynotfound event before handling accesskeys.
>      WidgetKeyboardEvent* nativeKeyEvent =
>        aKeyEvent->WidgetEventPtr()->AsKeyboardEvent();
>      if (nativeKeyEvent && nativeKeyEvent->mAccessKeyForwardedToChild) {

It would be better to bail out here (at line 214) and just return NS_OK if nativeKeyEvent was null, as this is a situation that should never happen anyway.

Comment 5

[Andi [:andi]]

Comment on attachment 8758220 [details]
Bug 1276901 - prevent null pointer dereference on |nativeKeyEvent|.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56542/diff/3-4/

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8758220 [details]
Bug 1276901 - prevent null pointer dereference on |nativeKeyEvent|.

https://reviewboard.mozilla.org/r/56542/#review86274

This seems fine, particularly given that Enn has already taken a look (I suspect he may be more familiar with XUL + event-handling internals than I am).

r+ with the following addressed:

::: layout/xul/nsMenuBarListener.cpp:214
(Diff revision 4)
> -    if (nativeKeyEvent && nativeKeyEvent->mAccessKeyForwardedToChild) {
> +    if (!nativeKeyEvent || nativeKeyEvent &&
> +      nativeKeyEvent->mAccessKeyForwardedToChild) {

Two nits:
(1) indentation is off on the second line of the "if" check.
(2) It'd be better to wrap the "&&" expression in a layer of parenthesis, to make it clearer where the logical grouping happens.

So, let's replace this with:
    if (!nativeKeyEvent ||
        (nativeKeyEvent && nativeKeyEvent->mAccessKeyForwardedToChild)) {


(fixing the indentation and making the grouping clearer)

::: layout/xul/nsMenuBarListener.cpp:255
(Diff revision 4)
>          mAccessKeyDown = mAccessKeyDownCanceled = false;
>  
>          aKeyEvent->StopPropagation();
>          aKeyEvent->PreventDefault();
>        }
> -    }    
> +    }

Please revert this whitespace-fix.  (If you'd like to clean up this whitespcae, please make the fix in a separate whiteespace-only patch.)

Because: we shouldn't mix non-contextual whitespace cleanup into a patch that is making actual functional changes.

Comment 7

[Andi [:andi]]

Attachment: Bug 1276901 - prevent null pointer dereference on |nativeKeyEvent|.

Review commit: https://reviewboard.mozilla.org/r/90064/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/90064/

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8806713 [details]
Bug 1276901 - prevent null pointer dereference on |nativeKeyEvent|.

https://reviewboard.mozilla.org/r/90064/#review89662

r=me

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9af43b01f022
prevent null pointer dereference on |nativeKeyEvent|. r=dholbert

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9af43b01f022