1304504 - Check the return value of GetIntPref() properly or an initialized bogus value may be used

Status: RESOLVED FIXED

(Core :: Audio/Video: MediaStreamGraph, defect, P2)

Description

[ISHIKAWA, Chiaki]

Attachment: VideoConduit-unused-result.patch

In media/webrtc/signaling/src/media-conduit/VideoConduit.cpp,
I found the return value of |GetIntPref()| is not properly checked, and if the call fails, the value of |temp| which is picked up from the stack, if it contains a random positive value, may be used to fill in other variables.
(Or for that matter, in the series of GetIntPref() succeeds once, that the fetched value may be used to fill in other values unintentionally when subsequent calls to |GetIntPref()| fails(?).

BAD.

This, despite the use of |NS_WARN_IF()| macro, of which return value is ignored sadly.

Here I propose a change that checks the return value properly, and use the returned value only when the call to |GetIntPref()| succeeds.


The code here was changed in Bug 1004396 - Make peerconnection video bitrates configurable. So I am requesting a review from  Eric Rescorla who reviewed the previous patch.

TIa

Comment 1

[Eric Rescorla (:ekr)]

Comment on attachment 8793481 [details] [diff] [review]
VideoConduit-unused-result.patch

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

Redirecting to Jesup.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8793481 [details] [diff] [review]
VideoConduit-unused-result.patch

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

r+ with nits.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +281,1 @@
>        }

An empty if doesn't help anything.  Use either (void) NS_WARN_IF, or Unused << NS_WARN_IF

@@ +281,5 @@
>        }
> +      if (NS_WARN_IF(NS_FAILED(branch->GetIntPref("media.peerconnection.video.min_bitrate", &temp))))
> +         ;
> +      else
> +      {

Again, avoid empty if's... if (!NS_WARN_IF(...)) { .... }

@@ +300,5 @@
> +      else
> +      {
> +        if (temp >= 0) {
> +          mMaxBitrate = temp;
> +        }

or simply set temp = -1 before each GetIntPref, and then this won't touch it - but the if (!NS_WARN_IF(...)) is probably cleaner

@@ -308,4 @@
>        }
>      }
>    }
> -

don't remove the blank line

Comment 3

[ISHIKAWA, Chiaki]

Attachment: VideoConduit-unused-result.patch carrying over r=rjesup+

(In reply to Randell Jesup [:jesup] from comment #2)
> Comment on attachment 8793481 [details] [diff] [review]
> VideoConduit-unused-result.patch
> 
> Review of attachment 8793481 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with nits.
> 
> ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
> @@ +281,1 @@
> >        }
> 
> An empty if doesn't help anything.  Use either (void) NS_WARN_IF, or Unused
> << NS_WARN_IF
> 

Took care of this.
A couple of things to note:

(void) NS_WARN_IF does not remove the compiler error.

Unused << NS_WARN_IF  does.

However, I needed to include "mozilla/Unused.h" since |Unused| is declared in the header file.

> @@ +281,5 @@
> >        }
> > +      if (NS_WARN_IF(NS_FAILED(branch->GetIntPref("media.peerconnection.video.min_bitrate", &temp))))
> > +         ;
> > +      else
> > +      {
> 
> Again, avoid empty if's... if (!NS_WARN_IF(...)) { .... }
> 

Taken care of.

> @@ +300,5 @@
> > +      else
> > +      {
> > +        if (temp >= 0) {
> > +          mMaxBitrate = temp;
> > +        }
> 
> or simply set temp = -1 before each GetIntPref, and then this won't touch it
> - but the if (!NS_WARN_IF(...)) is probably cleaner

I believe it is cleaner and more explicit about the error/situation where the
said preference does not exist.
 
> @@ -308,4 @@
> >        }
> >      }
> >    }
> > -
> 
> don't remove the blank line

Taken care of.


The patch that addresses the above comment is uploaded now.

Comment 4

[ISHIKAWA, Chiaki]

I put the checkin-needed keyword. 
TIA

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/50732fa0cafc
Check the return value of GetIntPref() properly. r+=rjesup

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/50732fa0cafc