GNOME Bugzilla – Bug 796185
v4l2object: Lacks some locking in gst_v4l2_object_get_property_helper
Last modified: 2018-06-27 21:16:34 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.
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.
Wrong bugzilla entry in last entry, it is this one: https://bugzilla.gnome.org/show_bug.cgi?id=795941
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.
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.
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.
With your patch, I am not able to reproduce the bug of this bug report anymore.
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).
Attachment 372137 [details] pushed as f72c713 - v4l2object: Don't open the device in get property