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 745443 - v4l2: fix fd leak in dma_buf import mode
v4l2: fix fd leak in dma_buf import mode
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
unspecified
Other Linux
: Normal normal
: 1.4.6
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-03-02 11:08 UTC by Gwenole Beauchesne
Modified: 2015-03-02 20:41 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
v4l2allocator: fix fd leak in DMABUF import mode (893 bytes, patch)
2015-03-02 11:09 UTC, Gwenole Beauchesne
needs-work Details | Review
v4l2allocator: fix fd leak in DMABUF import mode (944 bytes, patch)
2015-03-02 14:03 UTC, Gwenole Beauchesne
none Details | Review
v4l2allocator: fix fd leak in DMABUF import mode (1.28 KB, patch)
2015-03-02 15:35 UTC, Gwenole Beauchesne
committed Details | Review

Description Gwenole Beauchesne 2015-03-02 11:08:25 UTC
The attached patch fixes an fd leak when e.g. v4l2src io-mode=dmabuf-import is used. The reason behind this is that we were re-assigning the parent memory group fd with a new fd where the old one was not closed first.
Comment 1 Gwenole Beauchesne 2015-03-02 11:09:11 UTC
Created attachment 298270 [details] [review]
v4l2allocator: fix fd leak in DMABUF import mode

BTW, I want to push this one to 1.4 branch too. Thanks.
Comment 2 Nicolas Dufresne (ndufresne) 2015-03-02 14:01:25 UTC
Review of attachment 298270 [details] [review]:

You are missing bug link in your commit message.
Comment 3 Gwenole Beauchesne 2015-03-02 14:03:56 UTC
Created attachment 298299 [details] [review]
v4l2allocator: fix fd leak in DMABUF import mode

The patch was created before the report and the new "attachment" feature was used during the creation of the said bug. :)
Comment 4 Nicolas Dufresne (ndufresne) 2015-03-02 14:11:30 UTC
Review of attachment 298270 [details] [review]:

This patch is wrong. The code dup() the FD and you close it right away. Then you give the driver a closed FD ...
Comment 5 Nicolas Dufresne (ndufresne) 2015-03-02 14:11:54 UTC
Review of attachment 298299 [details] [review]:

Same.
Comment 6 Nicolas Dufresne (ndufresne) 2015-03-02 14:15:46 UTC
gst_v4l2_allocator_clear_dmabufin() is supposed to be called before importing again.
Comment 7 Gwenole Beauchesne 2015-03-02 14:17:47 UTC
(In reply to Nicolas Dufresne (stormer) from comment #4)
> Review of attachment 298270 [details] [review] [review]:
> 
> This patch is wrong. The code dup() the FD and you close it right away. Then
> you give the driver a closed FD ...

No, please read again, with the context.
Comment 8 Gwenole Beauchesne 2015-03-02 14:21:55 UTC
(In reply to Nicolas Dufresne (stormer) from comment #6)
> gst_v4l2_allocator_clear_dmabufin() is supposed to be called before
> importing again.

Should we also gst_v4l2_allocator_clear_userptr (); while we are it for USERPTR as well?
Comment 9 Nicolas Dufresne (ndufresne) 2015-03-02 15:00:32 UTC
We do clear on DQ. Which means we close the FD when we get back the buffer from the driver. Closing an FD in import make no sense, the buffer should have been cleared. Your bug is elsewhere.

Can you provide me with a way to reproduce. Should be reproducible with vivid imho.
Comment 10 Gwenole Beauchesne 2015-03-02 15:35:51 UTC
Created attachment 298316 [details] [review]
v4l2allocator: fix fd leak in DMABUF import mode
Comment 11 Nicolas Dufresne (ndufresne) 2015-03-02 20:40:55 UTC
Thanks.

Master:
commit 8cd40e77b70dae2a7b76f41c47a753a526d4da49

1.4
commit fe3cdccbf2922053fe3774875e315d5ed9a66a72
Author: Gwenole Beauchesne <gwenole.beauchesne@intel.com>
Date:   Mon Mar 2 12:04:00 2015 +0100

    v4l2allocator: fix fd leak in DMABUF import mode.
    
    Ensure gst_v4l2_buffer_pool_release_buffer() releases the associated
    GstV4l2MemoryGroup. In particular, this allows for closing the DMABUF
    handles prior to instantiating new ones.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=745443