[Automated review] Coverity thinks a pointer is potentially null if after a null-check that breaks a loop
Categories
(Developer Infrastructure :: Source Code Analysis, defect, P2)
Tracking
(Not tracked)
People
(Reporter: hsivonen, Assigned: andi)
References
(Blocks 1 open bug)
Details
https://phabricator.services.mozilla.com/D30873
Unrelated to the changes in the patch, the file being patched, nsFocusManager.cpp
has code like this
while (1) {
nsIFrame* frame = iterStartContent->GetPrimaryFrame();
// if there is no frame, look for another content node that has a frame
while (!frame) {
// if the root content doesn't have a frame, just return
if (iterStartContent == aRootContent) {
return NS_OK;
}
// look for the next or previous content node in tree order
iterStartContent = aForward ? iterStartContent->GetNextNode()
: iterStartContent->GetPreviousContent();
if (!iterStartContent) {
break;
}
frame = iterStartContent->GetPrimaryFrame();
Coverity thinks that iterStartContent
may be null on the last line quoted above despite the break
immediately above in the null case.
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 2•6 years ago
|
||
Obviously this is a serious problem with the way how the analyzer looks at iterStartContent
, it's like it doesn't understand how container nsCOMPtr
behaves. Here we can find more details about the issue in order to understand why the analyzer disregard the logic block where we break from the while
loop.
From the above mention artifact we can see the last 2 stack events:
19
line_number 3330
description "Member function call \"iterStartContent\" returns field \"mRawPtr\"."
file_path "/builds/worker/checkouts…base/nsFocusManager.cpp"
path_type "identity_transfer"
20
line_number 3330
description "Dereferencing a pointer that might be \"nullptr\" \"iterStartContent\" when calling \"GetPrimaryFrame\"."
file_path "/builds/worker/checkouts/gecko/dom/base/nsFocusManager.cpp"
path_type "dereference"
This definitely looks like a bug in Coverity, but I think we have two solutions for this:
- I plan on updating this week to the latest analyzer version 2019.4
- I will pass the modeling file to the review-time analyzer, in order to better understand
MOZ_ASSERT
since in the above case the nullptr is guarderd as such, https://searchfox.org/mozilla-central/source/xpcom/base/nsCOMPtr.h#842
Updated•6 years ago
|
Comment 3•5 years ago
|
||
The priority flag is not set for this bug.
:sylvestre, could you have a look please?
For more information, please visit auto_nag documentation.
Updated•5 years ago
|
Assignee | ||
Comment 4•5 years ago
|
||
This has been fixed in the latest reviewbot updates for the coverity
try job.
Updated•2 years ago
|
Description
•