[Static Analysis][Dereference after null check] In function nsDocument::GetViewportInfo from nsDocument.cpp

RESOLVED FIXED in Firefox 46

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: andi, Assigned: andi)

Tracking

(Blocks: 1 bug, {coverity})

Trunk
mozilla46
coverity
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

(Whiteboard: CID 1324682 )

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

2 years ago
The Static Analysis tool Coverity added that a null pointer derefence mai occure before after null check for context, it is checked here:

>>  float fullZoom = context ? context->DeviceContext()->GetFullZoom() : 1.0;
>>  fullZoom = (fullZoom == 0.0) ? 1.0 : fullZoom;

and dereferenced here:

>>  CSSToLayoutDeviceScale layoutDeviceScale = context->CSSToDevPixelScale();

This is not normal because in the case context is null we will haev a null dereference. I suggest removing the line, since variable layoutDeviceScale  is not used in the current context.
(Assignee)

Comment 1

2 years ago
(In reply to Bogdan Postelnicu from comment #0)
> The Static Analysis tool Coverity added that a null pointer derefence mai
> occure before after null check for context, it is checked here:
> 
> >>  float fullZoom = context ? context->DeviceContext()->GetFullZoom() : 1.0;
> >>  fullZoom = (fullZoom == 0.0) ? 1.0 : fullZoom;
> 
> and dereferenced here:
> 
> >>  CSSToLayoutDeviceScale layoutDeviceScale = context->CSSToDevPixelScale();
> 
> This is not normal because in the case context is null we will haev a null
> dereference. I suggest removing the line, since variable layoutDeviceScale 
> is not used in the current context.

We can modify layoutDeviceScale with:

CSSToLayoutDeviceScale layoutDeviceScale = context ? CSSToLayoutDeviceScale(1) : context->CSSToDevPixelScale();
(Assignee)

Comment 2

2 years ago
Created attachment 8701067 [details] [diff] [review]
Bug 1234554.diff
Attachment #8701067 - Flags: review?(jst)
(Assignee)

Comment 3

2 years ago
Created attachment 8701068 [details] [diff] [review]
Bug 1234554.diff
Attachment #8701067 - Attachment is obsolete: true
Attachment #8701067 - Flags: review?(jst)
Attachment #8701068 - Flags: review?(jst)
Comment on attachment 8701068 [details] [diff] [review]
Bug 1234554.diff

>-  CSSToLayoutDeviceScale layoutDeviceScale = context->CSSToDevPixelScale();
>+  CSSToLayoutDeviceScale layoutDeviceScale = context ? CSSToLayoutDeviceScale(1) : context->CSSToDevPixelScale();

That check is backwards, no? Or am I missing something obvious?

r- until we sort that out. Thanks!
Attachment #8701068 - Flags: review?(jst) → review-
(Assignee)

Comment 5

2 years ago
Created attachment 8701363 [details] [diff] [review]
Bug 1234554.diff

Yes completely backwards! I've attached the right one.
Attachment #8701068 - Attachment is obsolete: true
Attachment #8701363 - Flags: review?(jst)
Comment on attachment 8701363 [details] [diff] [review]
Bug 1234554.diff

That makes much more sense! :) r=jst
Attachment #8701363 - Flags: review?(jst) → review+
(Assignee)

Comment 7

2 years ago
Thanks for pointing the obvious :)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/88a2ffb5920e
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox46: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.