1111299 - Use of uninitialised value in mozilla::PeerConnectionImpl::ConfigureJsepSessionCodecs

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[mitchwharper]

Attachment: Valgrind trace

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:37.0) Gecko/20100101 Firefox/37.0
Build ID: 20141210144331

Steps to reproduce:

Running Valgrind on a trunk build with configure flags `--disable-tests '--enable-optimize=-g -O -freorder-blocks' --disable-jemalloc --enable-valgrind` 

and valgrind command 

`valgrind --tool=memcheck --trace-children=yes --vex-iropt-register-updates=allregs-at-mem-access  --smc-check=all-non-file --track-origns=yes ./firefox`

With steps

Steps taken:
1. Start the browser
2. Open a new tab
3. Visit https://www.webrtc-experiment.com/RTCMultiConnection/MultiRTC/ in two separate tabs
4. Input the same room ID for both instances
5. Enable video and audio on the second tab, and allow access
6. Share my microphone and camera
7. Switch to other tab
8. Enable video and audio on first tab
9. Share camera and microphone
10. Preview camera from second user


Actual results:

The first complaint:

==3330== Thread 1 Web Content:
==3330== Conditional jump or move depends on uninitialised value(s)
==3330==    at 0x62BF254: mozilla::PeerConnectionImpl::ConfigureJsepSessionCodecs() (dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp#906)

906  if (preferredCodec) {

Which later leads to a use at (http://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp#759)

754    explicit CompareCodecPriority(int32_t preferredCodec) {
755      // This pref really ought to be a string, preferably something like
756      // "H264" or "VP8" instead of a payload type.
757      // Bug 1101259.
758      std::ostringstream os;
759      os << preferredCodec;
760      mPreferredCodec = os.str();
761    }

Comment 1

[Daniel Veditz [:dveditz]]

Needs a security rating... do we make disastrous memory unsafe choices based on this, or just garble some data?

Comment 2

[Byron Campen [:bwc]]

So we end up with some random integer as the prioritized payload type, which may or may not cause some codec to be prioritized when it should not have been. This could lead to sub-optimal media quality because we preferred a poor codec, but there doesn't seem to be any security risk.

Comment 3

[Byron Campen [:bwc]]

Attachment: Make sure we don't prioritize random codecs.

Comment 4

[Byron Campen [:bwc]]

Comment on attachment 8537342 [details] [diff] [review]
Make sure we don't prioritize random codecs.

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

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=44a397daab6e

Comment 5

[Byron Campen [:bwc]]

Can someone mark this as not a sec bug, since there's no security risk?

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ea524040974

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7ea524040974