Closed Bug 1095251 Opened 10 years ago Closed 10 years ago

Determine what keeps MediaDecoder alive in GStreamer callbacks

Categories

(Core :: Audio/Video, defect)

36 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: karlt, Assigned: karlt)

References

Details

(Keywords: csectype-uaf, sec-audit, Whiteboard: [adv-main38-])

Attachments

(2 files)

MDReaders have a bare pointer to their MediaDecoders, which are used in GStreamer callbacks. If this pointer is cleared in MediaDecoderReader::BreakCycles(), immediately before MediaDecoderStateMachine releases its reference to the MediaDecoder in BreakCycles(), then mochitests crash in GStreamerReader::ReadAndPushData() GStreamerReader::Eos() trying to use mDecoder. MediaDecoderReader::BreakCycles() { if (mSampleDecodedCallback) { mSampleDecodedCallback->BreakCycles(); mSampleDecodedCallback = nullptr; } mTaskQueue = nullptr; + mDecoder = nullptr; } BreakCycles is very late in MDSM shutdown. I would have expected all decode threads to have completed at this point, or at least ceased assuming that the MediaDecoder still exists.
A typical stack trace, with source files having minor changes from 58e4264903ba is #1 0x00007ff61782f5d8 in mozilla::GStreamerReader::SeekData ( this=0x7ff578a5c800, aSrc=0x7ff5d4273c50, aOffset=83700) at /mnt/ssd1/karl/moz/dev/dom/media/gstreamer/GStreamerReader.cpp:977 #2 0x00007ff61782f58b in mozilla::GStreamerReader::SeekDataCb ( aSrc=0x7ff5d4273c50, aOffset=83700, aUserData=0x7ff578a5c800) at /mnt/ssd1/karl/moz/dev/dom/media/gstreamer/GStreamerReader.cpp:969 #3 0x00007ff5949062a8 in gst_app_src_create () from /usr/lib64/libgstapp-0.10.so.0 #4 0x00007ff5946d451e in gst_base_src_get_range () from /usr/lib64/libgstbase-0.10.so.0 #5 0x00007ff5946d56d1 in gst_base_src_pad_get_range () from /usr/lib64/libgstbase-0.10.so.0 #6 0x00007ff594b6a4c7 in gst_pad_get_range_unchecked () from /usr/lib64/libgstreamer-0.10.so.0 #7 0x00007ff594b6f3e9 in gst_pad_pull_range () from /usr/lib64/libgstreamer-0.10.so.0 #8 0x00007ff594b6a4c7 in gst_pad_get_range_unchecked () from /usr/lib64/libgstreamer-0.10.so.0 #9 0x00007ff594b6f3e9 in gst_pad_pull_range () from /usr/lib64/libgstreamer-0.10.so.0 #10 0x00007ff59377041f in gst_type_find_element_getrange () from /usr/lib64/gstreamer-0.10/libgstcoreelements.so #11 0x00007ff594b6a4c7 in gst_pad_get_range_unchecked () from /usr/lib64/libgstreamer-0.10.so.0 #12 0x00007ff594b6f3e9 in gst_pad_pull_range () from /usr/lib64/libgstreamer-0.10.so.0 #13 0x00007ff5897ac11a in gst_qtdemux_pull_atom () from /usr/lib64/gstreamer-0.10/libgstisomp4.so #14 0x00007ff5897c1526 in gst_qtdemux_loop () from /usr/lib64/gstreamer-0.10/libgstisomp4.so #15 0x00007ff594b94906 in gst_task_func () from /usr/lib64/libgstreamer-0.10.so.0 #16 0x00007ff60fc0348e in g_thread_pool_thread_proxy () from /usr/lib64/libglib-2.0.so.0 #17 0x00007ff60fc02add in g_thread_proxy () from /usr/lib64/libglib-2.0.so.0 #18 0x00007ff61dc6227a in start_thread () from /lib64/libpthread.so.0 #19 0x00007ff61cf7167d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:111
The reason the decoder is still alive is that nsDecoderDisposeEvent releases its reference to mStateMachine (on the main thread) before its reference to mDecoder, mStateMachine holds the last reference to the reader, and ~GStreamerReader joins GStreamer threads that run callbacks. I'd like to clean up mPlayBin earlier than ~GStreamerReader in case something still holds a reference to the reader, which has a bare pointer to the decoder, which is separately owned. #0 pthread_cond_wait () at ../nptl/sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S:185 #1 0x00007ff60fc20a98 in g_cond_wait () from /usr/lib64/libglib-2.0.so.0 #2 0x00007ff594b956f5 in gst_task_join () from /usr/lib64/libgstreamer-0.10.so.0 #3 0x00007ff594b70afa in gst_pad_stop_task () from /usr/lib64/libgstreamer-0.10.so.0 #4 0x00007ff593760597 in gst_queue_src_activate_push () from /usr/lib64/gstreamer-0.10/libgstcoreelements.so #5 0x00007ff594b6dc9b in gst_pad_activate_push () from /usr/lib64/libgstreamer-0.10.so.0 #6 0x00007ff594b6e890 in gst_pad_set_active () from /usr/lib64/libgstreamer-0.10.so.0 #7 0x00007ff594b4f8f1 in activate_pads () from /usr/lib64/libgstreamer-0.10.so.0 #8 0x00007ff594b612cd in gst_iterator_fold () from /usr/lib64/libgstreamer-0.10.so.0 #9 0x00007ff594b4f853 in iterator_activate_fold_with_resync () from /usr/lib64/libgstreamer-0.10.so.0 #10 0x00007ff594b51e86 in gst_element_pads_activate () from /usr/lib64/libgstreamer-0.10.so.0 #11 0x00007ff594b52215 in gst_element_change_state_func () from /usr/lib64/libgstreamer-0.10.so.0 #12 0x00007ff594b53efc in gst_element_change_state () from /usr/lib64/libgstreamer-0.10.so.0 #13 0x00007ff594b54832 in gst_element_set_state_func () from /usr/lib64/libgstreamer-0.10.so.0 #14 0x00007ff594b413d3 in gst_bin_change_state_func () from /usr/lib64/libgstreamer-0.10.so.0 #15 0x00007ff594b53efc in gst_element_change_state () from /usr/lib64/libgstreamer-0.10.so.0 #16 0x00007ff594b54832 in gst_element_set_state_func () from /usr/lib64/libgstreamer-0.10.so.0 #17 0x00007ff594b413d3 in gst_bin_change_state_func () from /usr/lib64/libgstreamer-0.10.so.0 #18 0x00007ff593dd5e7c in gst_play_sink_change_state () from /usr/lib64/gstreamer-0.10/libgstplaybin.so #19 0x00007ff594b53efc in gst_element_change_state () from /usr/lib64/libgstreamer-0.10.so.0 #20 0x00007ff594b54832 in gst_element_set_state_func () from /usr/lib64/libgstreamer-0.10.so.0 #21 0x00007ff594b413d3 in gst_bin_change_state_func () from /usr/lib64/libgstreamer-0.10.so.0 #22 0x00007ff594b722f3 in gst_pipeline_change_state () from /usr/lib64/libgstreamer-0.10.so.0 #23 0x00007ff593dcf48d in gst_play_bin_change_state () from /usr/lib64/gstreamer-0.10/libgstplaybin.so #24 0x00007ff594b53efc in gst_element_change_state () from /usr/lib64/libgstreamer-0.10.so.0 #25 0x00007ff594b53f7f in gst_element_change_state () from /usr/lib64/libgstreamer-0.10.so.0 #26 0x00007ff594b54832 in gst_element_set_state_func () from /usr/lib64/libgstreamer-0.10.so.0 #27 0x00007ff61782c732 in mozilla::GStreamerReader::~GStreamerReader ( this=0x7ff578a5c800, __in_chrg=<optimized out>) at /mnt/ssd1/karl/moz/dev/dom/media/gstreamer/GStreamerReader.cpp:125 #28 0x00007ff61782c822 in mozilla::GStreamerReader::~GStreamerReader ( this=0x7ff578a5c800, __in_chrg=<optimized out>) at /mnt/ssd1/karl/moz/dev/dom/media/gstreamer/GStreamerReader.cpp:139 #29 0x00007ff6176d30ff in mozilla::MediaDecoderReader::Release ( this=0x7ff578a5c800) at ../../dist/include/MediaDecoderReader.h:61 #30 0x00007ff6176f95e1 in nsRefPtr<mozilla::MediaDecoderReader>::assign_assuming_AddRef (this=0x7ff578cf2de8, aNewPtr=0x0) at ../../dist/include/nsRefPtr.h:47 #31 0x00007ff6176ee9dc in nsRefPtr<mozilla::MediaDecoderReader>::assign_with_AddRef (this=0x7ff578cf2de8, aRawPtr=0x0) at ../../dist/include/nsRefPtr.h:31 #32 0x00007ff6176e27ab in nsRefPtr<mozilla::MediaDecoderReader>::operator= ( this=0x7ff578cf2de8, aRhs=0x0) at ../../dist/include/nsRefPtr.h:127 #33 0x00007ff6176a1d25 in mozilla::MediaDecoderStateMachine::~MediaDecoderStateMachine (this=0x7ff578cf2c00, __in_chrg=<optimized out>) at /mnt/ssd1/karl/moz/dev/dom/media/MediaDecoderStateMachine.cpp:287 #34 0x00007ff6176a1e1a in mozilla::MediaDecoderStateMachine::~MediaDecoderStateMachine (this=0x7ff578cf2c00, __in_chrg=<optimized out>) at /mnt/ssd1/karl/moz/dev/dom/media/MediaDecoderStateMachine.cpp:292 #35 0x00007ff61768518b in mozilla::MediaDecoderStateMachine::Release ( this=0x7ff578cf2c00) at /mnt/ssd1/karl/moz/dev/dom/media/MediaDecoderStateMachine.h:127 #36 0x00007ff6176f8013 in nsRefPtr<mozilla::MediaDecoderStateMachine>::assign_assuming_AddRef (this=0x7ff5cedae180, aNewPtr=0x0) at ../../dist/include/nsRefPtr.h:47 #37 0x00007ff6176ecd76 in nsRefPtr<mozilla::MediaDecoderStateMachine>::assign_with_AddRef (this=0x7ff5cedae180, aRawPtr=0x0) at ../../dist/include/nsRefPtr.h:31 #38 0x00007ff6176e0811 in nsRefPtr<mozilla::MediaDecoderStateMachine>::operator= (this=0x7ff5cedae180, aRhs=0x0) at ../../dist/include/nsRefPtr.h:127 #39 0x00007ff6176d4d0e in mozilla::nsDecoderDisposeEvent::Run ( this=0x7ff5cedae160) at /mnt/ssd1/karl/moz/dev/dom/media/MediaDecoderStateMachine.cpp:2687
Assignee: nobody → karlt
instead of during reader destruction. Closing the guts of the reader during Shutdown() is consistent with what is done in WebMReader and MP4Reader.
Attachment #8562282 - Flags: review?(edwin)
My main concern here is the dangling pointer. If we know that the state machine holds the last reference to the reader, then there is no need to reference count the reader. If the reader is reference counted then we should assume that something else may hold a reference to the reader and so we should clear dangling pointers. There's probably more than one place where the decoder pointer could be set to null, MDReader::BreakCycles() being the other primary candidate. The choice of Shutdown() here is consistent with what was done with TrackBuffer's subreaders. Subreaders can be Shutdown() while their task queue (the parent reader's) continues to process tasks, so clearing the decoder on the task queue (instead of the main thread) provides memory consistency should there be a need for a subsequent task to null-check the pointer before using it. I'm not convinced that mShutdown is always true in ~MediaDecoderReader, particularly with some of the set-up failure paths in webaudio/MediaBufferDecoder.cpp. But if mShutdown is true, then mDecoder should be null. I guess we could still ensure that Shutdown() is called when reader set-up fails and permit such a call on a non-decode thread when there is no task queue, but I'm treating that as an existing issue and leaving as is for now.
Attachment #8562283 - Flags: review?(cpearce)
Attachment #8562283 - Flags: review?(cpearce) → review+
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Depends on: 1137076
This "fixed" a potential security issue in the last iteration of the fix for bug 1069602. However, the security issue is now replaced with a null dereference, tracked in bug 1137076. https://bugzilla.mozilla.org/attachment.cgi?oldid=8497922&action=interdiff&newid=8498694&headers=1 When ProcessCachedData() runs, there is no lock held. After the IsShutdown() test succeeds, mIsShutdown may be set to true on the main thread before the bare pointer mDecoder is dereferenced. If mIsShutdown is true, the object associated with mDecoder may have been deleted. At least the pointer is now null when dereferenced.
Depends on: 1139874
Whiteboard: [adv-main38-]
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.

Attachment

General

Created:
Updated:
Size: