GNOME Bugzilla – Bug 727409
streamsynchronizer: Invalid memory accesses when using uncopyable memory
Last modified: 2014-08-01 12:24:18 UTC
if streamsynchronizer receives a buffer which is not copyable then it can cause a crash. gststreamsynchronizer.c line 556 and 557: buffer = gst_buffer_make_writable (buffer); GST_BUFFER_FLAG_UNSET (buffer, GST_BUFFER_FLAG_DISCONT); The code does not check the return value of gst_buffer_make_writable () in my case, I have buffers with an uncopyable GstMemory which causes gst_mini_object_make_writable to return NULL. Here's what valgrind shows: 0:00:20.032290552 25081 0x7ea1780 DEBUG GST_PERFORMANCE gstminiobject.c:326:gst_mini_object_make_writable: copy GstBuffer miniobject 0x4f8a828 -> (nil) ==25081== Thread 10: ==25081== Invalid read of size 4 ==25081== at 0x51586F4: gst_stream_synchronizer_sink_chain (gststreamsynchronizer.c:557) ==25081== by 0x48B0717: gst_pad_push_data (gstpad.c:3760) ==25081== by 0x48A0173: gst_proxy_pad_chain_default (gstghostpad.c:128) ==25081== by 0x48B0717: gst_pad_push_data (gstpad.c:3760) ==25081== by 0x5342A1F: gst_selector_pad_chain (gstinputselector.c:1108) ==25081== by 0x48B0717: gst_pad_push_data (gstpad.c:3760) ==25081== by 0x48A0173: gst_proxy_pad_chain_default (gstghostpad.c:128) ==25081== by 0x48B0717: gst_pad_push_data (gstpad.c:3760) ==25081== by 0x48A0173: gst_proxy_pad_chain_default (gstghostpad.c:128) ==25081== by 0x48B0717: gst_pad_push_data (gstpad.c:3760) ==25081== by 0x523625F: gst_video_decoder_clip_and_push_buf (gstvideodecoder.c:2657) ==25081== by 0x523BBDF: gst_video_decoder_finish_frame (gstvideodecoder.c:2572) ==25081== Address 0xc is not stack'd, malloc'd or (recently) free'd ==25081== Caught SIGSEGV Notice the (nil) printed by gst_mini_object_make_writable() Here is my pipeline: GST_DEBUG='*:3,GST_PERFORMANCE:5' valgrind gst-launch-1.0 playbin uri=file:////home/nemo/Videos/Camera/20140327_001.mp4 flags=99
I'm not convinced it is "legal" to disallow the copy operation. Lots of existing code assumes the buffer returned by _make_writable will be valid-and-non-NULL. Do you have a particular reason for believing this is valid ?
That's perfectly legal, yes. Memory can be un-copyable, unmappable, unwritable, etc. Just having such a memory will limit the usefulness of it a lot. streamsynchronizer should just error out here if that happens. OTOH it's entirely not clear to me why we drop DISCONT flags on the first buffer here. My commit 4 years ago does not really explain it and I can only remember something about gapless audio playback, where the audio sinks will cause a short click sound if they see a DISCONT on the first buffer of a new stream. Should be investigated if this code is still necessary or if it can be solved in a better way.
Does this imply all code should test for the return value of gst_buffer_make_writable to be NULL and error out accordingly ? The vast majority of the code does not at the moment.
Yes, that's like most code not checking the return value of gst_buffer_map() (well, a lot of code changed in that regard over the last months).
There used to be a gst_buffer_make_metadata_writable, it seems to have gone in 1.x. If a buffer cannot be copied, can it be read ? If it cannot be read, what is the use of it ? Some kind of "lazy evaluated" buffer ? If it can be read, why can it not be copied ? I find this weird, but maybe I'm looking at it the wrong way :)
EGLImage buffers are generally not possible to be read and also can't be copied, but the sink can still render them.
EGLImage buffers are an example for a non-copyable memory. Another example would be Android gralloc handles (they can be copied but it's expensive). I am not sure why playbin does not crash when we use egl-allocated memory though. @Sebastian if the audio sinks complain and we have to unset the DISCONT flag then why are we seeing the issue here? I am passing video buffers. Emitting an error would destroy a zero-copy video rendering pipeline so I don't really think it's a good idea (If I dare to do so about a core developer's idea!). I guess we can stop dropping the DISCONT flag from video buffers (Do our sinks really care?) which will bypass the copy call completely. We also need to check for the return value for audio just in case and send an error message,
Yes, I think we should get rid of the unsetting of the DISCONT flag completely here (not just for video). It doesn't make any sense and is only a workaround for a different problem.
Created attachment 282098 [details] [review] proposed fix I checked GstGlMemory and it implements memory copying (that's a mem copy which is IMHO not good). dmabuf uses the default copy implementation (I am not sure where that is). I take back my words that this bug destroys zero copy playback because the first buffer (or only DISCONT buffers) will be copied. We can also get away without fixing this bug but we should update the documentation to state that the allocator should implement copying if it is to be used for playback related elements. Anyway I am attaching a patch that drops the buffer copy operation.
commit b34e0ba91cc01511c395c1d4ee44ec1ccb8df165 Author: Mohammed Sameer <msameer@foolab.org> Date: Wed Jul 30 20:53:53 2014 +0300 streamsynchronizer: don't unset DISCONT flag Unsetting DISCONT flag means we need to copy the buffer. This copy operation mandates that all GstMemory should be copy-able which is not always the case https://bugzilla.gnome.org/show_bug.cgi?id=727409