GNOME Bugzilla – Bug 784302
v4l2allocator leak when dmabuf creation fails
Last modified: 2017-06-28 21:04:27 UTC
When the v4l2allocator allocates a dmabuf, I'm observing what appears to be a memory leak when the dmabuf creation process fails. Specifically, when duplicating the dmabuf file descriptor fails (i.e. control passes to the dup_failed: label in gst_v4l2_allocator_alloc_dmabuf()), the allocator itself has an extra reference placed on it which is never removed, resulting in the allocator later failing to be cleaned up when the pipeline shuts down. I was able to resolve the issue by placing a call to gst_object_unref (allocator) in the dup_failed block. However, I am not positive that this is the correct fix, as I'm having some trouble understanding the refcounting semantics of the allocator through all of the GstMemory gymnastics that are taking place here. I'm also unclear whether the expbuf_failed case would need the same fix, in which case the unref should go in the cleanup: label or even in the _cleanup_failed_alloc() function. Can someone advise on exactly what the correct solution to this issue would be?
Could you share what you did, might be better to understand what you tried.
Created attachment 354643 [details] [review] Fix memory leak in dmabuf allocation Sure. Here's what I tried, and it seems to resolve the error. But I'm confused about how the refcounting works in here, so I'm not sure if this is the right place to fix it or not.
Review of attachment 354643 [details] [review]: ::: sys/v4l2/gstv4l2allocator.c @@ +922,3 @@ cleanup: { + gst_object_unref (allocator); I'm not sure if it's fully correct, the leak is in fact cause by the fact the we leave before we increment mems_allocated. I think the right solution is just to move "group->mems_allocated++;" right after: > /* Take back the allocator reference */ > gst_object_ref (allocator); > } -> HERE Would you mind testing it ? Otherwise you also leak the GstMemory object.
Created attachment 354646 [details] [review] Fix memory leak in dmabuf allocation I nearly suggested that originally, but wasn't sure if that would cause some kind of other consequences. That does appear to fix the issue for me, so here's an updated patch.
Thanks. This was pushed in master as e8a46787388ae7e1ee037304fd984758d39f1627. It's a good candiate for backport, though I'll give it some more testing myself before.