Conditional jump or move depends on uninitialised value(s) at mozilla::GStreamerReader::NotifyDataArrived

RESOLVED FIXED in Firefox 36

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: mitchwharper, Assigned: eflores)

Tracking

({csectype-uninitialized, valgrind})

34 Branch
mozilla37
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox35 wontfix, firefox36 fixed, firefox37 fixed, firefox-esr31 unaffected)

Details

(Whiteboard: [adv-main36-])

Attachments

(1 attachment)

Reporter

Description

5 years ago
User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0
Build ID: 20141126041045



Actual results:

==6262== Conditional jump or move depends on uninitialised value(s)
==6262==    at 0x9259CA7: mozilla::GStreamerReader::NotifyDataArrived(char const*, unsigned int, long) (GStreamerReader.cpp:1240)
==6262==    by 0x91D21BC: mozilla::MediaDecoderStateMachine::NotifyDataArrived(char const*, unsigned int, long) (MediaDecoderStateMachine.cpp:1478)
==6262==    by 0x91C5BA5: mozilla::MediaDecoder::NotifyDataArrived(char const*, unsigned int, long) (MediaDecoder.cpp:1525)
==6262==    by 0x91D7987: mozilla::ChannelMediaResource::CopySegmentToCache(nsIInputStream*, void*, char const*, unsigned int, unsigned int, unsigned int*) (MediaResource.cpp:507)
==6262==    by 0x7D9F09F: nsPipeInputStream::ReadSegments(tag_nsresult (*)(nsIInputStream*, void*, char const*, unsigned int, unsigned int, unsigned int*), void*, unsigned int, unsigned int*) (nsPipe3.cpp:822)
==6262==    by 0x91D8AC7: mozilla::ChannelMediaResource::OnDataAvailable(nsIRequest*, nsIInputStream*, unsigned int) (MediaResource.cpp:545)
==6262==    by 0x91D8B1E: mozilla::ChannelMediaResource::Listener::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) (MediaResource.cpp:141)
==6262==    by 0x91604F5: mozilla::dom::HTMLMediaElement::MediaLoadListener::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) (HTMLMediaElement.cpp:390)
==6262==    by 0x7E192F7: nsBaseChannel::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) (nsBaseChannel.cpp:781)
==6262==    by 0x7E26D6A: nsInputStreamPump::OnStateTransfer() (nsInputStreamPump.cpp:609)
==6262==    by 0x7E27230: nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) (nsInputStreamPump.cpp:436)
==6262==    by 0x7DA1313: nsInputStreamReadyEvent::Run() (nsStreamUtils.cpp:88)
==6262==  Uninitialised value was created by a heap allocation
==6262==    at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==6262==    by 0x40334E6: moz_xmalloc (mozalloc.cpp:52)
==6262==    by 0x9258510: mozilla::GStreamerDecoder::CreateStateMachine() (mozalloc.h:201)
==6262==    by 0x91C6619: mozilla::MediaDecoder::Load(nsIStreamListener**, mozilla::MediaDecoder*) (MediaDecoder.cpp:550)
==6262==    by 0x9164DE4: mozilla::dom::HTMLMediaElement::FinishDecoderSetup(mozilla::MediaDecoder*, mozilla::MediaResource*, nsIStreamListener**, mozilla::MediaDecoder*) (HTMLMediaElement.cpp:2680)
==6262==    by 0x916504D: mozilla::dom::HTMLMediaElement::InitializeDecoderForChannel(nsIChannel*, nsIStreamListener**) (HTMLMediaElement.cpp:2630)
==6262==    by 0x9165FB8: mozilla::dom::HTMLMediaElement::MediaLoadListener::OnStartRequest(nsIRequest*, nsISupports*) (HTMLMediaElement.cpp:349)
==6262==    by 0x7E1945F: nsBaseChannel::OnStartRequest(nsIRequest*, nsISupports*) (nsBaseChannel.cpp:737)
==6262==    by 0x7E26B86: nsInputStreamPump::OnStateStart() (nsInputStreamPump.cpp:531)
==6262==    by 0x7E27181: nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) (nsInputStreamPump.cpp:433)
==6262==    by 0x7DA1313: nsInputStreamReadyEvent::Run() (nsStreamUtils.cpp:88)
==6262==    by 0x7DB34B3: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:823)
Reporter

Updated

5 years ago
OS: Windows 8 → Linux
Bobby, I hear you're doing media code now - can you have a look?
Component: Untriaged → Video/Audio
Flags: needinfo?(bobbyholley)
Product: Firefox → Core
(In reply to :Gijs Kruitbosch from comment #1)
> Bobby, I hear you're doing media code now - can you have a look?

No, sorry. I don't know anything about the gstreamer code. Over to Anthony to find an owner.

BTW - Please don't take my security background and current media work as a reason to flag me for fixing arbitrary media security bugs. I'm here to help ship MSE, and don't plan to own this code.
Flags: needinfo?(bobbyholley) → needinfo?(ajones)
(In reply to Bobby Holley (:bholley) from comment #2)
> (In reply to :Gijs Kruitbosch from comment #1)
> > Bobby, I hear you're doing media code now - can you have a look?
> 
> No, sorry. I don't know anything about the gstreamer code. Over to Anthony
> to find an owner.
> 
> BTW - Please don't take my security background and current media work as a
> reason to flag me for fixing arbitrary media security bugs. I'm here to help
> ship MSE, and don't plan to own this code.

Sorry! I'll keep it in mind.

For reference, it looks like this and all the other bugs filed by Mitch today came out of the investigation in bug 1108455.
Keywords: valgrind
Reporter

Comment 4

5 years ago
Valgrind command: `G_SLICE=always-malloc valgrind --tool=memcheck --vex-iropt-register-updates=allregs-at-mem-access --smc-check=all-non-file ./firefox`

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 (this is where the first jump on uninitialized memory occured)
11. Preview microphone from second user
12. Switch tabs
13. Preview camera and mic from first user
14. Exit browser
I don't understand where gstreamer is related to webrtc.
Flags: needinfo?(ajones) → needinfo?(mreavy)
Reporter

Comment 6

5 years ago
It may not be directly related to webrtc, only steps 1-3 are required for this to occur (it happens every time the page is opened). When you load the page, it says it establishes a data connection with the server. This all happens before getUserMedia() is ever called.
Flags: needinfo?(ajones)
This is not getUserMedia or WebRTC related.
Flags: needinfo?(mreavy)
Flags: needinfo?(ajones)
Ralph - can you look into this?
Flags: needinfo?(giles)
Attachment #8536910 - Flags: review?(ajones) → review+
This seems pretty benign -- either we'll call UpdateEstimatedDuration() when we didn't need to, or we don't call it when we should have. The WebAudio version of that is an empty function even, not sure which of the three variants would be called here. Anthony, do you know?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(ajones)
I'm happy to look at it, but it looks like edwin already has a fix.
Flags: needinfo?(giles)
Ralph - looks like edwin has it under control.
I say we land the patch on nightly and move on.
Flags: needinfo?(ajones) → needinfo?(giles)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/eb6e806e0bbd
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Comment on attachment 8536910 [details] [diff] [review]
1109540.patch

Approval Request Comment
[Feature/regressing bug #]: 422540
[User impact if declined]: Users exposed to security issue. This isn't an MSE bug, but we'd like to keep the media code in sync to simplify backporting other changes.
[Describe test coverage new/current, TBPL]: Landed on m-c.
[Risks and why]: Low; this just avoid using uninitialized data when starting playback.
[String/UUID change made/needed]: None.
Attachment #8536910 - Flags: approval-mozilla-aurora?
Attachment #8536910 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
How far back does this go?
Flags: needinfo?(edwin)
Comment on attachment 8536910 [details] [diff] [review]
1109540.patch

(see approval request in comment 16)

Goes all the way down to release, though not really exploitable AFAICT. Should be fine without it.

May as well uplift to beta though.
Flags: needinfo?(edwin)
Attachment #8536910 - Flags: approval-mozilla-beta?
Comment on attachment 8536910 [details] [diff] [review]
1109540.patch

We released 35 and beta is now 36.
Attachment #8536910 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Whiteboard: [adv-main36-]

Updated

4 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.