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 796185 - v4l2object: Lacks some locking in gst_v4l2_object_get_property_helper
v4l2object: Lacks some locking in gst_v4l2_object_get_property_helper
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
1.14.0
Other Linux
: Normal major
: 1.14.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2018-05-16 22:36 UTC by Marie Maurer
Modified: 2018-06-27 21:16 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
v4l2object: Don't open the device in get property (1.29 KB, patch)
2018-05-17 00:18 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review

Description Marie Maurer 2018-05-16 22:36:55 UTC
I was debugging a problem with a playback pipeline on am i.MX6,
with Gstreamer 1.14.

I am using v4l2h264dec to decode data and generating debug output,
including DOT files in certain situations. Sometimes the pipeline fails,
e.g. by

onGstMessageError Could not dup device '/dev/video15' for reading and writing., v4l2_calls.c(758): gst_v4l2_dup (): /GstPipeline:Player/v4l2h264dec:v4l2h264dec4:
system error: Bad file descriptor

onGstMessageError Could not initialize supporting library., gstvideodecoder.c(2535): gst_video_decoder_change_state (): /GstPipeline:Player/v4l2h264dec:v4l2h264dec4:
Failed to open decoder

After some debugging I think I know the problem: There are two threads,
accessing the same device (my decoder). One is my normal decoding of the video.
The other seems to be gst_v4l2_object_get_property_helper, case PROP_DEVICE_NAME.

After further trying to narrow the error, I was able to switch on/off the
problem by enabling/disabling DOT files generation. The generation of
DOT files seems to be using gst_v4l2_object_get_property_helper, case PROP_DEVICE_NAME, to get the device name, like CODA960 or imx-ipu-vout.

The code in gst_v4l2_object_get_property_helper, case PROP_DEVICE_NAME
(see here:
https://github.com/GStreamer/gst-plugins-good/blob/master/sys/v4l2/gstv4l2object.c#L727 ) seems to check if file is open, if not, opens it,
doing something (getting the name) and closing it. When in parallel
(exactly parallel?) the decoder wants to open the files, or is in process
of opening the file, it seems to be a race condition that decoder cannot
be opening or is closed shortly afterwards by gst_v4l2_close setting video_fd
to -1.

I cannot fix it, sorry, too less knowledge in this environment, 
but the current code seems to be buggy. It also looks like open the device
to read the device name again and again seems to be inefficient.
Can it read only once and caching it? The case PROP_DEVICE in same function
looks easier and different.
Comment 1 Marie Maurer 2018-05-16 22:41:31 UTC
The reported error varies: Sometimes the above error message, then report
that empty CAPS were detected:

gstv4l2videodec.c:145:gst_v4l2_video_dec_open:<v4l2h264dec5>[00m error: Encoder on device /dev/video15 has no supported input format

      Line 5557: 0:11:06.141285746 [334m  381[00m 0x6c646a60 [36mINFO   [00m [00;01;31;47m    GST_ERROR_SYSTEM gstelement.c:2145:gst_element_message_full_with_details:<v4l2h264dec5>[00m posting message: Encoder on device /dev/video15 has no supported input format

Pay attention: String "Encoder" was meanhwile changed to "Decoder"
on gstreamer master branch. See https://bugzilla.gnome.org/show_bug.cgi?id=795586
for details.
Comment 2 Marie Maurer 2018-05-16 22:45:01 UTC
Wrong bugzilla entry in last entry, it is this one:
https://bugzilla.gnome.org/show_bug.cgi?id=795941
Comment 3 Nicolas Dufresne (ndufresne) 2018-05-17 00:12:07 UTC
Clearly this function lacks some locking, I was not aware of what this function was doing, I don't really like it. It's a bit pointless, I'd simply return NULL if the object is not open yet.
Comment 4 Nicolas Dufresne (ndufresne) 2018-05-17 00:18:10 UTC
Created attachment 372137 [details] [review]
v4l2object: Don't open the device in get property

This is both racy and inefficient. This function is still missing some
locking which will be address in later patch.
Comment 5 Nicolas Dufresne (ndufresne) 2018-05-17 00:19:10 UTC
This should fix it, but locking is still required, so that if the element is set to NULL state while calling g_value_set_string() we don't get a use after free.
Comment 6 Marie Maurer 2018-05-17 14:03:10 UTC
With your patch, I am not able to reproduce the bug of this bug report anymore.
Comment 7 Nicolas Dufresne (ndufresne) 2018-05-17 15:11:11 UTC
Make sense, the bug now falls into theoretical land, I'll probably merge this if there isn't any objection around dropping this "feature". But I'd really like to keep this bug open until I have proper locking added (and tested).
Comment 8 Nicolas Dufresne (ndufresne) 2018-05-28 16:59:10 UTC
Attachment 372137 [details] pushed as f72c713 - v4l2object: Don't open the device in get property