Closed
Bug 1234554
Opened 8 years ago
Closed 8 years ago
[Static Analysis][Dereference after null check] In function nsDocument::GetViewportInfo from nsDocument.cpp
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: andi, Assigned: andi)
References
(Blocks 1 open bug)
Details
(Keywords: coverity, Whiteboard: CID 1324682 )
Attachments
(1 file, 2 obsolete files)
1.34 KB,
patch
|
jst
:
review+
|
Details | Diff | Splinter Review |
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•8 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•8 years ago
|
||
Attachment #8701067 -
Flags: review?(jst)
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8701067 -
Attachment is obsolete: true
Attachment #8701067 -
Flags: review?(jst)
Attachment #8701068 -
Flags: review?(jst)
Comment 4•8 years ago
|
||
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•8 years ago
|
||
Yes completely backwards! I've attached the right one.
Attachment #8701068 -
Attachment is obsolete: true
Attachment #8701363 -
Flags: review?(jst)
Comment 6•8 years ago
|
||
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•8 years ago
|
||
Thanks for pointing the obvious :)
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 9•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/88a2ffb5920e
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•