Fix uchex warnings about null-check after pointer deref in webrtc/signaling

RESOLVED FIXED in Firefox 50

Status

()

P2
normal
Rank:
25
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: cpeterson, Assigned: cpeterson)

Tracking

unspecified
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Created attachment 8762937 [details] [diff] [review]
webrtc-signaling-uchex.patch

These warnings were reported by µchex, 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)

https://web.stanford.edu/%7Emlfbrown/paper.pdf

These functions null check their pointer parameters, implying the parameters could legitimately be null, but they also dereference the pointers *before* the null checks.

These warnings are not actual bugs today because none of these functions' current callers pass pointers that could be null. (They're passing the addresses of local stack variables.) The SDP coding style includes many superfluous null checks of pointer parameters, which triggers these uchex warnings. These null checks could be removed or replaced with debug asserts.
Attachment #8762937 - Flags: review?(adam)
Rank: 25
Priority: -- → P2
Comment on attachment 8762937 [details] [diff] [review]
webrtc-signaling-uchex.patch

Review of attachment 8762937 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
LGTM
Attachment #8762937 - Flags: review?(adam) → review+
(In reply to Chris Peterson [:cpeterson] from comment #0)
> The SDP coding style includes many
> superfluous null checks of pointer parameters, which triggers these uchex
> warnings. These null checks could be removed or replaced with debug asserts.

If you could provide us with a list of these that would be great.
(Assignee)

Comment 3

2 years ago
(In reply to Nils Ohlmeier [:drno] from comment #2)
> (In reply to Chris Peterson [:cpeterson] from comment #0)
> > The SDP coding style includes many
> > superfluous null checks of pointer parameters, which triggers these uchex
> > warnings. These null checks could be removed or replaced with debug asserts.
> 
> If you could provide us with a list of these that would be great.

next_token() is a good example. It's a static function within sdp_utils.c so all its callers are known. Its parameter checks seem overly defensive and verbose for a function (though mostly harmless) that's not a public API.

http://searchfox.org/mozilla-central/source/media/webrtc/signaling/src/sdp/sipcc/sdp_utils.c#69

Some other examples:

http://searchfox.org/mozilla-central/source/media/webrtc/signaling/src/sdp/sipcc/sdp_utils.c#325
http://searchfox.org/mozilla-central/source/media/webrtc/signaling/src/sdp/sipcc/sdp_utils.c#355
http://searchfox.org/mozilla-central/source/media/webrtc/signaling/src/sdp/sipcc/sdp_utils.c#440

Comment 4

2 years ago
Pushed by cpeterson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cdb479ad755
Fix uchex warnings about null-check after pointer deref in webrtc/signaling. r=drno

Comment 5

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6cdb479ad755
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.