1273423 - [Static Analysis][Dereference after null check] In function nsNativeThemeGTK::GetGtkWidgetAndState

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Andi [:andi]]

The Static Analysis tool Coverity added that variable |aFrame| is null checked here:

>>    if (!aFrame) {
>>      // reset the entire struct to zero
>>      memset(aState, 0, sizeof(GtkWidgetState));
>>    } else 

and dereferenced later on:

>>  case NS_THEME_RANGE:
>>    {
>>      if (IsRangeHorizontal(aFrame)) {
>>        if (aWidgetFlags)
>>          *aWidgetFlags = GTK_ORIENTATION_HORIZONTAL;
>>        aGtkWidgetType = MOZ_GTK_SCALE_HORIZONTAL;
>>      }

this implies that a potential null pointer dereference can happen

Comment 1

[Andi [:andi]]

Attachment: MozReview Request: Bug 1273423 - removed useless null pointer check on |aFrame|. r?[e-mail]

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

Comment 2

[Karl Tomlinson (:karlt)]

Comment on attachment 8753262 [details]
MozReview Request: Bug 1273423 - removed useless null pointer check on |aFrame|. r?[e-mail]

https://reviewboard.mozilla.org/r/53154/#review50186

(In reply to Andi-Bogdan Postelnicu from comment #0)
> this implies that a potential null pointer dereference can happen

Either that or the null check in the first case is unnecessary
(or even that it is unnecessary when aWidgetType is NS_THEME_RANGE).

But looking through the callers, and documentation, there is no evidence that
aFrame may be null.

The null check dates back to the first version of the file [1] and I don't
think it is necessary now (if it ever was).

nsNativeThemeCocoa.mm and nsNativeThemeWin.cpp also seem to assume that aFrame
is non-null.

So I think the best fix for the inconsistency detected by Coverity is to
remove the dead code for !aFrame.  Automated testing will tell us if my
analysis is incorrect.

http://52.25.115.98/viewvc/main/mozilla/widget/src/gtk2/nsNativeThemeGTK.cpp?annotate=1.32#l239

Comment 3

[Andi [:andi]]

Comment on attachment 8753262 [details]
MozReview Request: Bug 1273423 - removed useless null pointer check on |aFrame|. r?[e-mail]

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

Comment 4

[Karl Tomlinson (:karlt)]

Comment on attachment 8753262 [details]
MozReview Request: Bug 1273423 - removed useless null pointer check on |aFrame|. r?[e-mail]

https://reviewboard.mozilla.org/r/53154/#review50464

Thanks!  Please update the commit message for the new change.

Comment 5

[Andi [:andi]]

Comment on attachment 8753262 [details]
MozReview Request: Bug 1273423 - removed useless null pointer check on |aFrame|. r?[e-mail]

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

Comment 6

[Andi [:andi]]

Attachment: Bug 1273423 - removed useless null pointer check on |aFrame|. [e-mail]

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

Comment 7

[Karl Tomlinson (:karlt)]

Comment on attachment 8775966 [details]
Bug 1273423 - removed useless null pointer check on |aFrame|. [e-mail]

https://reviewboard.mozilla.org/r/67956/#review65274

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f0727a3518c6
removed useless null pointer check on |aFrame|. r=[e-mail]

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f0727a3518c6

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Not worth uplifting it I guess