Closed
Bug 1278452
Opened 8 years ago
Closed 8 years ago
Fix null-check-after-deref issues found in C++ code with µchex
Categories
(Core :: General, defect)
Core
General
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(16 files)
838 bytes,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
1.36 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
1.35 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
2.14 KB,
patch
|
u480271
:
review+
|
Details | Diff | Splinter Review |
2.54 KB,
patch
|
u480271
:
review+
|
Details | Diff | Splinter Review |
913 bytes,
patch
|
karlt
:
review+
|
Details | Diff | Splinter Review |
1.39 KB,
patch
|
valentin
:
review+
|
Details | Diff | Splinter Review |
1.26 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
1.90 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
2.41 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
1.58 KB,
patch
|
masayuki
:
review+
|
Details | Diff | Splinter Review |
1.59 KB,
patch
|
karlt
:
review+
|
Details | Diff | Splinter Review |
1001 bytes,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
1.48 KB,
patch
|
mattwoodrow
:
review+
|
Details | Diff | Splinter Review |
4.65 KB,
patch
|
lsalzman
:
review+
|
Details | Diff | Splinter Review |
1.40 KB,
patch
|
ayang
:
review+
|
Details | Diff | Splinter Review |
ChexMix is a static analysis tool described in this paper: How to Build Static Checking Systems Using Orders of Magnitude Less Code Fraser Brown, Andres Nötzli, Dawson Engler ASPLOS '16 I've been in contact with the authors and they've given me access to the result of an up-to-date run of the null-check-after-deref issues found. I will land fixes via this bug.
Assignee | ||
Comment 1•8 years ago
|
||
More specifically, the tool identifies cases where you deref a pointer and then subsequently null check it. This often indicates an unnecessary null check, or a misplaced null check. It also sometimes indicates a typo, such as using |p| instead of |*p|.
Assignee | ||
Comment 2•8 years ago
|
||
Attachment #8760562 -
Flags: review?(sphink)
Assignee | ||
Comment 3•8 years ago
|
||
The check is unnecessary because |operator new| is infallible.
Attachment #8760566 -
Flags: review?(mrbkap)
Assignee | ||
Comment 4•8 years ago
|
||
mLineContainer is dereferenced shortly before and after the check, so it's obviously not useful.
Attachment #8760567 -
Flags: review?(cam)
Updated•8 years ago
|
Attachment #8760562 -
Flags: review?(sphink) → review+
Assignee | ||
Comment 5•8 years ago
|
||
|audio| and |video| are both dereferenced beforehand.
Attachment #8760569 -
Flags: review?(dglastonbury)
Assignee | ||
Comment 6•8 years ago
|
||
The patch also avoids printing the surface format twice in some error messages.
Attachment #8760570 -
Flags: review?(dglastonbury)
Assignee | ||
Comment 7•8 years ago
|
||
The fixes in bug 1278439 were also inspired by one issue reported by this tool.
Assignee | ||
Comment 8•8 years ago
|
||
|operator new| is infallible now.
Attachment #8760571 -
Flags: review?(karlt)
Assignee | ||
Comment 9•8 years ago
|
||
Attachment #8760574 -
Flags: review?(valentin.gosu)
Assignee | ||
Comment 10•8 years ago
|
||
|aLanguage| must be dereferenced when checked.
Attachment #8760575 -
Flags: review?(ehsan)
Assignee | ||
Comment 11•8 years ago
|
||
Attachment #8760579 -
Flags: review?(mrbkap)
Updated•8 years ago
|
Attachment #8760566 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 12•8 years ago
|
||
Attachment #8760583 -
Flags: review?(nfroyd)
Assignee | ||
Comment 13•8 years ago
|
||
Attachment #8760584 -
Flags: review?(continuation)
Updated•8 years ago
|
Attachment #8760579 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 14•8 years ago
|
||
Attachment #8760586 -
Flags: review?(karlt)
Assignee | ||
Comment 15•8 years ago
|
||
There's another null check just a little earlier.
Attachment #8760597 -
Flags: review?(nfroyd)
Assignee | ||
Comment 16•8 years ago
|
||
Attachment #8760603 -
Flags: review?(matt.woodrow)
Updated•8 years ago
|
Attachment #8760571 -
Flags: review?(karlt) → review+
Updated•8 years ago
|
Attachment #8760586 -
Flags: review?(karlt) → review+
Updated•8 years ago
|
Attachment #8760603 -
Flags: review?(matt.woodrow) → review+
Updated•8 years ago
|
Attachment #8760574 -
Flags: review?(valentin.gosu) → review+
Updated•8 years ago
|
Attachment #8760583 -
Flags: review?(nfroyd) → review+
Updated•8 years ago
|
Attachment #8760597 -
Flags: review?(nfroyd) → review+
Updated•8 years ago
|
Attachment #8760584 -
Flags: review?(continuation) → review?(masayuki)
Updated•8 years ago
|
Attachment #8760584 -
Flags: review?(masayuki) → review+
Updated•8 years ago
|
Attachment #8760575 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 17•8 years ago
|
||
(It turns that that Chexmix was a working name for the tool. It's now called µchex.)
Summary: Fix null-check-after-deref issues found with ChexMix → Fix null-check-after-deref issues found with µchex
Updated•8 years ago
|
Attachment #8760567 -
Flags: review?(cam) → review+
Attachment #8760569 -
Flags: review?(dglastonbury) → review+
Attachment #8760570 -
Flags: review?(dglastonbury) → review+
Assignee | ||
Comment 18•8 years ago
|
||
Attachment #8761018 -
Flags: review?(lsalzman)
Updated•8 years ago
|
Attachment #8761018 -
Flags: review?(lsalzman) → review+
Comment 19•8 years ago
|
||
Upstream Skia bug report: https://codereview.chromium.org/2046563007/
Assignee | ||
Comment 20•8 years ago
|
||
|aData| can be null, because FindAndRemoveBufferHolder() can return null.
Attachment #8761039 -
Flags: review?(ayang)
Updated•8 years ago
|
Attachment #8761039 -
Flags: review?(ayang) → review+
Assignee | ||
Updated•8 years ago
|
Summary: Fix null-check-after-deref issues found with µchex → Fix null-check-after-deref issues found in C++ code with µchex
Comment hidden (me-too) |
Assignee | ||
Comment 22•8 years ago
|
||
https://ssl.icu-project.org/trac/ticket/12390 is a upstream report for an ICU issue.
Assignee | ||
Comment 23•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d63042e0d38c658599b0770aa6f821ef8d59ad8 Bug 1278452 - Remove an unnecessary null check in js/src/shell/. r=sfink. https://hg.mozilla.org/integration/mozilla-inbound/rev/4b3df77d399ad1be7c9df0987a0e1f0c9226a2d4 Bug 1278452 - Remove an unnecessary null check in js/src/shell/. r=mrbkap. https://hg.mozilla.org/integration/mozilla-inbound/rev/dd5cfca074c36cdb5aa323ff53b03539650bec1f Bug 1278452 - Remove an unnecessary null check in layout/generic/. r=heycam. https://hg.mozilla.org/integration/mozilla-inbound/rev/588913c4442f3406d5fbb4f0ea9f93cc03004334 Bug 1278452 - Remove unnecessary null checks in dom/media/. r=kamidphish. https://hg.mozilla.org/integration/mozilla-inbound/rev/85e0b6798f9025d77e6926892d7ae712e5c94ff6 Bug 1278452 - Move a null check earlier in ConvertSourceSurfaceToNV12(). r=kamidphish. https://hg.mozilla.org/integration/mozilla-inbound/rev/42ff71805e2b67d5dabcd6b49ff0f737074bb91b Bug 1278452 - Remove unnecessary null checks in widget/gtk/. r=karlt. https://hg.mozilla.org/integration/mozilla-inbound/rev/d57948ecf695801b2950dd70c3340e91440102bc Bug 1278452 - Avoid a possible null deref in netwerk/base/. r=valentin. https://hg.mozilla.org/integration/mozilla-inbound/rev/ac0df3cacb3bf8356a9c9ca15c803b512b7de12e Bug 1278452 - Fix a typo in mozEnglishWordUtils::GetLanguage. r=ehsan. https://hg.mozilla.org/integration/mozilla-inbound/rev/9b6d23819097bb067fd9bea9b9d45df0f7fdc062 Bug 1278452 - Fix some null checks in dom/xbl/. r=mrbkap. https://hg.mozilla.org/integration/mozilla-inbound/rev/45ea273f14d1f1e17c352d7a5df4f1deff43283e Bug 1278452 - Add some null checks when getting streams. r=froydnj. https://hg.mozilla.org/integration/mozilla-inbound/rev/2202233b5b183143c85ab7a65b84a90003c262b3 Bug 1278452 - Avoid a null deref in dom/events/. r=mccr8. https://hg.mozilla.org/integration/mozilla-inbound/rev/cf7ac0a043f9d45fc4c852e26d1d59e4d1ee7468 Bug 1278452 - Avoid a null deref in widget/. r=karlt. https://hg.mozilla.org/integration/mozilla-inbound/rev/ae2bafad2ec2e5a2de35bce1b5e8939e8d26cff5 Bug 1278452 - Remove a redundant null check in xpcom/glue/. r=froydnj. https://hg.mozilla.org/integration/mozilla-inbound/rev/d47307c8a3008eb1d1cc37353f8687bbf81e4321 Bug 1278452 - Move a misplaced null check in layout/forms/. r=mattwoodrow. https://hg.mozilla.org/integration/mozilla-inbound/rev/77a4d658b09a702e656980405e6ec65f557434bc Bug 1278452 - Avoid a null deref in dom/media/platforms/omx/. r=ayang. https://hg.mozilla.org/integration/mozilla-inbound/rev/3a6de98f92e604b3bb6d47e7d70c871c396bdc38 Bug 1278452 - Fix up bad null checks in Skia. r=lsalzman.
Comment 24•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9d63042e0d38 https://hg.mozilla.org/mozilla-central/rev/4b3df77d399a https://hg.mozilla.org/mozilla-central/rev/dd5cfca074c3 https://hg.mozilla.org/mozilla-central/rev/588913c4442f https://hg.mozilla.org/mozilla-central/rev/85e0b6798f90 https://hg.mozilla.org/mozilla-central/rev/42ff71805e2b https://hg.mozilla.org/mozilla-central/rev/d57948ecf695 https://hg.mozilla.org/mozilla-central/rev/ac0df3cacb3b https://hg.mozilla.org/mozilla-central/rev/9b6d23819097 https://hg.mozilla.org/mozilla-central/rev/45ea273f14d1 https://hg.mozilla.org/mozilla-central/rev/2202233b5b18 https://hg.mozilla.org/mozilla-central/rev/cf7ac0a043f9 https://hg.mozilla.org/mozilla-central/rev/ae2bafad2ec2 https://hg.mozilla.org/mozilla-central/rev/d47307c8a300 https://hg.mozilla.org/mozilla-central/rev/77a4d658b09a https://hg.mozilla.org/mozilla-central/rev/3a6de98f92e6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in
before you can comment on or make changes to this bug.
Description
•