Fix null-check-after-deref issues found in C++ code with µchex

RESOLVED FIXED in Firefox 50

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

Trunk
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(16 attachments)

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
kamidphish
: review+
Details | Diff | Splinter Review
2.54 KB, patch
kamidphish
: 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
: 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
(Assignee)

Description

2 years ago
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

2 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

2 years ago
Created attachment 8760562 [details] [diff] [review]
Remove an unnecessary null check in js/src/shell/
Attachment #8760562 - Flags: review?(sphink)
(Assignee)

Comment 3

2 years ago
Created attachment 8760566 [details] [diff] [review]
Remove an unnecessary null check in js/src/shell/

The check is unnecessary because |operator new| is infallible.
Attachment #8760566 - Flags: review?(mrbkap)
(Assignee)

Comment 4

2 years ago
Created attachment 8760567 [details] [diff] [review]
Remove an unnecessary null check in layout/generic/

mLineContainer is dereferenced shortly before and after the check, so it's
obviously not useful.
Attachment #8760567 - Flags: review?(cam)
Attachment #8760562 - Flags: review?(sphink) → review+
(Assignee)

Comment 5

2 years ago
Created attachment 8760569 [details] [diff] [review]
Remove unnecessary null checks in dom/media/

|audio| and |video| are both dereferenced beforehand.
Attachment #8760569 - Flags: review?(dglastonbury)
(Assignee)

Comment 6

2 years ago
Created attachment 8760570 [details] [diff] [review]
Move a null check earlier in ConvertSourceSurfaceToNV12()

The patch also avoids printing the surface format twice in some error messages.
Attachment #8760570 - Flags: review?(dglastonbury)
(Assignee)

Comment 7

2 years ago
The fixes in bug 1278439 were also inspired by one issue reported by this tool.
(Assignee)

Comment 8

2 years ago
Created attachment 8760571 [details] [diff] [review]
Remove unnecessary null checks in widget/gtk/

|operator new| is infallible now.
Attachment #8760571 - Flags: review?(karlt)
(Assignee)

Comment 9

2 years ago
Created attachment 8760574 [details] [diff] [review]
Avoid a possible null deref in netwerk/base/
Attachment #8760574 - Flags: review?(valentin.gosu)
(Assignee)

Comment 10

2 years ago
Created attachment 8760575 [details] [diff] [review]
Fix a typo in mozEnglishWordUtils::GetLanguage

|aLanguage| must be dereferenced when checked.
Attachment #8760575 - Flags: review?(ehsan)
(Assignee)

Comment 11

2 years ago
Created attachment 8760579 [details] [diff] [review]
Fix some null checks in dom/xbl/
Attachment #8760579 - Flags: review?(mrbkap)
Attachment #8760566 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 12

2 years ago
Created attachment 8760583 [details] [diff] [review]
Add some null checks when getting streams
Attachment #8760583 - Flags: review?(nfroyd)
(Assignee)

Comment 13

2 years ago
Created attachment 8760584 [details] [diff] [review]
Avoid a null deref in dom/events/
Attachment #8760584 - Flags: review?(continuation)
Attachment #8760579 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 14

2 years ago
Created attachment 8760586 [details] [diff] [review]
Avoid a null deref in widget/
Attachment #8760586 - Flags: review?(karlt)
(Assignee)

Comment 15

2 years ago
Created attachment 8760597 [details] [diff] [review]
Remove a redundant null check in xpcom/glue/

There's another null check just a little earlier.
Attachment #8760597 - Flags: review?(nfroyd)
(Assignee)

Comment 16

2 years ago
Created attachment 8760603 [details] [diff] [review]
Move a misplaced null check in layout/forms/
Attachment #8760603 - Flags: review?(matt.woodrow)
Attachment #8760571 - Flags: review?(karlt) → review+
Attachment #8760586 - Flags: review?(karlt) → review+
Attachment #8760603 - Flags: review?(matt.woodrow) → review+
Attachment #8760574 - Flags: review?(valentin.gosu) → review+
Attachment #8760583 - Flags: review?(nfroyd) → review+
Attachment #8760597 - Flags: review?(nfroyd) → review+
Attachment #8760584 - Flags: review?(continuation) → review?(masayuki)
Attachment #8760584 - Flags: review?(masayuki) → review+

Updated

2 years ago
Attachment #8760575 - Flags: review?(ehsan) → review+
(Assignee)

Comment 17

2 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
Attachment #8760567 - Flags: review?(cam) → review+
Attachment #8760569 - Flags: review?(dglastonbury) → review+
Attachment #8760570 - Flags: review?(dglastonbury) → review+
(Assignee)

Comment 18

2 years ago
Created attachment 8761018 [details] [diff] [review]
Fix up bad null checks in Skia
Attachment #8761018 - Flags: review?(lsalzman)
Attachment #8761018 - Flags: review?(lsalzman) → review+
(Assignee)

Comment 20

2 years ago
Created attachment 8761039 [details] [diff] [review]
Avoid a null deref in dom/media/platforms/omx/

|aData| can be null, because FindAndRemoveBufferHolder() can return null.
Attachment #8761039 - Flags: review?(ayang)
Attachment #8761039 - Flags: review?(ayang) → review+
(Assignee)

Updated

2 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

2 years ago
https://ssl.icu-project.org/trac/ticket/12390 is a upstream report for an ICU issue.
(Assignee)

Comment 23

2 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.
(Assignee)

Updated

2 years ago
Blocks: 1280844
You need to log in before you can comment on or make changes to this bug.