Closed Bug 1111299 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: WebRTC, defect)

37 Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox37 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: mitchwharper, Assigned: bwc)

Details

(Keywords: valgrind)

Attachments

(2 files)

Attached file 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    }
Keywords: valgrind
Attachment #8536123 - Attachment description: codecTrace.txt → Valgrind trace
Component: Untriaged → WebRTC
Product: Firefox → Core
Needs a security rating... do we make disastrous memory unsafe choices based on this, or just garble some data?
Flags: needinfo?(rjesup)
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.
Assignee: nobody → docfaraday
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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
Attachment #8537342 - Flags: review?(rjesup)
Attachment #8537342 - Flags: review?(rjesup) → review+
Can someone mark this as not a sec bug, since there's no security risk?

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ea524040974
https://hg.mozilla.org/mozilla-central/rev/7ea524040974
Group: core-security
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: needinfo?(rjesup)
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.