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

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Nicholas Nethercote [inactive]]

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.

Comment 1

[Nicholas Nethercote [inactive]]

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|.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Remove an unnecessary null check in js/src/shell/

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Remove an unnecessary null check in js/src/shell/

The check is unnecessary because |operator new| is infallible.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: Remove an unnecessary null check in layout/generic/

mLineContainer is dereferenced shortly before and after the check, so it's
obviously not useful.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: Remove unnecessary null checks in dom/media/

|audio| and |video| are both dereferenced beforehand.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: Move a null check earlier in ConvertSourceSurfaceToNV12()

The patch also avoids printing the surface format twice in some error messages.

Comment 7

[Nicholas Nethercote [inactive]]

The fixes in bug 1278439 were also inspired by one issue reported by this tool.

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: Remove unnecessary null checks in widget/gtk/

|operator new| is infallible now.

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: Avoid a possible null deref in netwerk/base/

Comment 10

[Nicholas Nethercote [inactive]]

Attachment: Fix a typo in mozEnglishWordUtils::GetLanguage

|aLanguage| must be dereferenced when checked.

Comment 11

[Nicholas Nethercote [inactive]]

Attachment: Fix some null checks in dom/xbl/

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: Add some null checks when getting streams

Comment 13

[Nicholas Nethercote [inactive]]

Attachment: Avoid a null deref in dom/events/

Comment 14

[Nicholas Nethercote [inactive]]

Attachment: Avoid a null deref in widget/

Comment 15

[Nicholas Nethercote [inactive]]

Attachment: Remove a redundant null check in xpcom/glue/

There's another null check just a little earlier.

Comment 16

[Nicholas Nethercote [inactive]]

Attachment: Move a misplaced null check in layout/forms/

Comment 17

[Nicholas Nethercote [inactive]]

(It turns that that Chexmix was a working name for the tool. It's now called µchex.)

Comment 18

[Nicholas Nethercote [inactive]]

Attachment: Fix up bad null checks in Skia

Comment 19

[Lee Salzman [:lsalzman]]

Upstream Skia bug report: https://codereview.chromium.org/2046563007/

Comment 20

[Nicholas Nethercote [inactive]]

Attachment: Avoid a null deref in dom/media/platforms/omx/

|aData| can be null, because FindAndRemoveBufferHolder() can return null.

Comment 21

[Nicholas Nethercote [inactive]]

https://codereview.chromium.org/2046563007/ is the upstream fix for the Skia issues.

Comment 22

[Nicholas Nethercote [inactive]]

https://ssl.icu-project.org/trac/ticket/12390 is a upstream report for an ICU issue.

Comment 23

[Nicholas Nethercote [inactive]]

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

[Carsten Book [:Tomcat]]

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