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

RESOLVED FIXED in Firefox 50

Status

()

defect
P2
normal
Rank:
25
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: cpeterson, Assigned: cpeterson)

Tracking

unspecified
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(1 attachment)

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.
(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
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
https://hg.mozilla.org/mozilla-central/rev/6cdb479ad755
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.