Closed Bug 1273423 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Widget: Gtk, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox49 --- wontfix
firefox51 --- fixed

People

(Reporter: andi, Assigned: andi)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 1361618)

Attachments

(2 files)

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 on attachment 8753262 [details]
MozReview Request: Bug 1273423 - removed useless null pointer check on |aFrame|. r?karlt+@karlt.net

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
Attachment #8753262 - Flags: review?(karlt)
Comment on attachment 8753262 [details]
MozReview Request: Bug 1273423 - removed useless null pointer check on |aFrame|. r?karlt+@karlt.net

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53154/diff/1-2/
Attachment #8753262 - Flags: review?(karlt)
Comment on attachment 8753262 [details]
MozReview Request: Bug 1273423 - removed useless null pointer check on |aFrame|. r?karlt+@karlt.net

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

Thanks!  Please update the commit message for the new change.
Attachment #8753262 - Flags: review?(karlt) → review+
Comment on attachment 8753262 [details]
MozReview Request: Bug 1273423 - removed useless null pointer check on |aFrame|. r?karlt+@karlt.net

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53154/diff/2-3/
Attachment #8753262 - Attachment description: MozReview Request: Bug 1273423 - prevent null pointer dereference on |aFrame|. r?karlt+@karlt.net → MozReview Request: Bug 1273423 - removed useless null pointer check on |aFrame|. r?karlt+@karlt.net
Attachment #8775966 - Flags: review?(karlt) → review+
Comment on attachment 8775966 [details]
Bug 1273423 - removed useless null pointer check on |aFrame|. +@karlt.net

https://reviewboard.mozilla.org/r/67956/#review65274
https://hg.mozilla.org/mozilla-central/rev/f0727a3518c6
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Not worth uplifting it I guess
You need to log in before you can comment on or make changes to this bug.