After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 766645 - matroskademux: don't hold object lock whilst pushing out headers, might lead to query deadlock
matroskademux: don't hold object lock whilst pushing out headers, might lead ...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal normal
: 1.8.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-05-19 06:58 UTC by Seungha Yang
Modified: 2016-05-19 21:07 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
matroskademux: Remove unnecessary lock condition (1.48 KB, patch)
2016-05-19 06:59 UTC, Seungha Yang
committed Details | Review
log file (421.01 KB, application/x-7z-compressed)
2016-05-19 07:38 UTC, Seungha Yang
  Details

Description Seungha Yang 2016-05-19 06:58:12 UTC
matroskademux: Remove unnecessary lock condition

matroska-demux has GST_OBJECT_LOCK in
- gst_matroska_demux_push_codec_data_all()
- gst_matroska_demux_query()

Some parse element such as FLAC checks upstream seekability, and
there is some use cases that matroska-demux is linked to a parse element
(e.g.,FLAC format) without intermediate elements (e.g., queue).
In this case, matroska-demux never get return of
gst_matroska_demux_push_codec_data_all(), because the parse element
can return it after the parse element receives return of upstream query.
Comment 1 Seungha Yang 2016-05-19 06:59:32 UTC
Created attachment 328170 [details] [review]
matroskademux: Remove unnecessary lock condition
Comment 2 Seungha Yang 2016-05-19 07:38:50 UTC
Created attachment 328172 [details]
log file

Attached log file was captured using following commend
gst-launch-1.0 filesrc location= mkv_H.264_FLAC.mkv ! matroskademux ! flacparse ! fakesink

This file is too large for uploading... sorry.
Comment 3 Tim-Philipp Müller 2016-05-19 20:03:56 UTC
Can be reproduced with:

gst-launch-1.0 audiotestsrc num-buffers=1000 ! flacenc ! matroskamux ! filesink location=/tmp/flac.mkv

gst-launch-1.0 filesrc location=/tmp/flac.mkv  ! matroskademux ! flacparse ! fakesink
Comment 4 Tim-Philipp Müller 2016-05-19 21:07:15 UTC
Thanks for the patch.

I don't see any reason why the demuxer needs to take the lock here either, the variables should be sufficiently protected by the stream lock.

And in general, elements should never hold the OBJECT_LOCK when pushing out buffers or events.

In addition to that, if the GST_ELEMENT_ERROR() code path was ever hit, that would deadlock too.

commit eb09829a1c1987373ae433daca6420ea6c0fb908
Author: Seungha Yang <sh.yang@lge.com>
Date:   Thu May 19 15:36:57 2016 +0900

    matroskademux: don't hold object lock whilst pushing out headers
    
    matroskademux would take the GST_OBJECT_LOCK in
    - gst_matroska_demux_push_codec_data_all()
    - gst_matroska_demux_query()
    
    Some parse element such as FLAC checks upstream seekability, and
    there is some use cases that matroska-demux is linked to a parse element
    (e.g.,FLAC format) without intermediate elements (e.g., queue).
    In this case, matroska-demux never returns from _push_codec_data_all()
    because the parser can return only after it receives the response to
    the upstream query, but that's not going to happen because it's
    deadlocked.
    
    Elements must not hold the object lock whilst pushing out events
    or data.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=766645