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

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect, P2)

Description

[Chris Peterson [:cpeterson]]

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

Comment 1

[Nils Ohlmeier [:drno]]

Comment on attachment 8762937 [details] [diff] [review]
webrtc-signaling-uchex.patch

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

Thanks!
LGTM

Comment 2

[Nils Ohlmeier [:drno]]

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

Comment 3

[Chris Peterson [:cpeterson]]

(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

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6cdb479ad755