[v3,2/2] gstreamer: allocator: gst_libcamera_allocator_new(): Fix memory leak
diff mbox series

Message ID 20250106100613.493108-2-pobrn@protonmail.com
State Accepted
Headers show
Series
  • [v3,1/2] gstreamer: allocator: gst_libcamera_allocator_new(): Recognize errors
Related show

Commit Message

Barnabás Pőcze Jan. 6, 2025, 10:06 a.m. UTC 
If `FrameBufferAllocator::allocate()` causes the construction to be
aborted, the allocated `GstLibcameraAllocator` will not be
deallocated properly. Use `g_autoptr()` to address this.

`g_steal_pointer()` could only be used in glib 2.68 or later because
earlier it evaluates to a pointer-to-void in C++, which would necessitate
a `static_cast`.

Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes in v3:
  - fix #include order

Changes in v2:
  - add `#include <utility>`
---
 src/gstreamer/gstlibcameraallocator.cpp | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

--
2.47.1

Comments

Kieran Bingham Jan. 9, 2025, 5:51 p.m. UTC | #1 
Quoting Barnabás Pőcze (2025-01-06 10:06:22)
> If `FrameBufferAllocator::allocate()` causes the construction to be
> aborted, the allocated `GstLibcameraAllocator` will not be
> deallocated properly. Use `g_autoptr()` to address this.
> 
> `g_steal_pointer()` could only be used in glib 2.68 or later because
> earlier it evaluates to a pointer-to-void in C++, which would necessitate
> a `static_cast`.
> 
> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

This looks ok to me, but I suspect Nicolas' review is more valuable than
mine on this one.


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> ---
> Changes in v3:
>   - fix #include order
> 
> Changes in v2:
>   - add `#include <utility>`
> ---
>  src/gstreamer/gstlibcameraallocator.cpp | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp
> index b0c84893a..d4492d994 100644
> --- a/src/gstreamer/gstlibcameraallocator.cpp
> +++ b/src/gstreamer/gstlibcameraallocator.cpp
> @@ -8,6 +8,8 @@
> 
>  #include "gstlibcameraallocator.h"
> 
> +#include <utility>
> +
>  #include <libcamera/camera.h>
>  #include <libcamera/framebuffer_allocator.h>
>  #include <libcamera/stream.h>
> @@ -199,15 +201,13 @@ GstLibcameraAllocator *
>  gst_libcamera_allocator_new(std::shared_ptr<Camera> camera,
>                             CameraConfiguration *config_)
>  {
> -       auto *self = GST_LIBCAMERA_ALLOCATOR(g_object_new(GST_TYPE_LIBCAMERA_ALLOCATOR,
> -                                                         nullptr));
> +       g_autoptr(GstLibcameraAllocator) self = GST_LIBCAMERA_ALLOCATOR(g_object_new(GST_TYPE_LIBCAMERA_ALLOCATOR,
> +                                                                                    nullptr));
>         gint ret;
> 
>         self->cm_ptr = new std::shared_ptr<CameraManager>(gst_libcamera_get_camera_manager(ret));
> -       if (ret) {
> -               g_object_unref(self);
> +       if (ret)
>                 return nullptr;
> -       }
> 
>         self->fb_allocator = new FrameBufferAllocator(camera);
>         for (StreamConfiguration &streamCfg : *config_) {
> @@ -228,7 +228,7 @@ gst_libcamera_allocator_new(std::shared_ptr<Camera> camera,
>                 g_hash_table_insert(self->pools, stream, pool);
>         }
> 
> -       return self;
> +       return std::exchange(self, nullptr);
>  }
> 
>  bool
> --
> 2.47.1
>
Nicolas Dufresne Jan. 9, 2025, 8:03 p.m. UTC | #2 
Hey,

Le jeudi 09 janvier 2025 à 17:51 +0000, Kieran Bingham a écrit :
> Quoting Barnabás Pőcze (2025-01-06 10:06:22)
> > If `FrameBufferAllocator::allocate()` causes the construction to be
> > aborted, the allocated `GstLibcameraAllocator` will not be
> > deallocated properly. Use `g_autoptr()` to address this.
> > 
> > `g_steal_pointer()` could only be used in glib 2.68 or later because
> > earlier it evaluates to a pointer-to-void in C++, which would necessitate
> > a `static_cast`.
> > 
> > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> This looks ok to me, but I suspect Nicolas' review is more valuable than
> mine on this one.
> 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Well, as requested:

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

> 
> > ---
> > Changes in v3:
> >   - fix #include order
> > 
> > Changes in v2:
> >   - add `#include <utility>`
> > ---
> >  src/gstreamer/gstlibcameraallocator.cpp | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp
> > index b0c84893a..d4492d994 100644
> > --- a/src/gstreamer/gstlibcameraallocator.cpp
> > +++ b/src/gstreamer/gstlibcameraallocator.cpp
> > @@ -8,6 +8,8 @@
> > 
> >  #include "gstlibcameraallocator.h"
> > 
> > +#include <utility>
> > +
> >  #include <libcamera/camera.h>
> >  #include <libcamera/framebuffer_allocator.h>
> >  #include <libcamera/stream.h>
> > @@ -199,15 +201,13 @@ GstLibcameraAllocator *
> >  gst_libcamera_allocator_new(std::shared_ptr<Camera> camera,
> >                             CameraConfiguration *config_)
> >  {
> > -       auto *self = GST_LIBCAMERA_ALLOCATOR(g_object_new(GST_TYPE_LIBCAMERA_ALLOCATOR,
> > -                                                         nullptr));
> > +       g_autoptr(GstLibcameraAllocator) self = GST_LIBCAMERA_ALLOCATOR(g_object_new(GST_TYPE_LIBCAMERA_ALLOCATOR,
> > +                                                                                    nullptr));
> >         gint ret;
> > 
> >         self->cm_ptr = new std::shared_ptr<CameraManager>(gst_libcamera_get_camera_manager(ret));
> > -       if (ret) {
> > -               g_object_unref(self);
> > +       if (ret)
> >                 return nullptr;
> > -       }
> > 
> >         self->fb_allocator = new FrameBufferAllocator(camera);
> >         for (StreamConfiguration &streamCfg : *config_) {
> > @@ -228,7 +228,7 @@ gst_libcamera_allocator_new(std::shared_ptr<Camera> camera,
> >                 g_hash_table_insert(self->pools, stream, pool);
> >         }
> > 
> > -       return self;
> > +       return std::exchange(self, nullptr);
> >  }
> > 
> >  bool
> > --
> > 2.47.1
> >

Patch
diff mbox series 

diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp
index b0c84893a..d4492d994 100644
--- a/src/gstreamer/gstlibcameraallocator.cpp
+++ b/src/gstreamer/gstlibcameraallocator.cpp
@@ -8,6 +8,8 @@ 

 #include "gstlibcameraallocator.h"

+#include <utility>
+
 #include <libcamera/camera.h>
 #include <libcamera/framebuffer_allocator.h>
 #include <libcamera/stream.h>
@@ -199,15 +201,13 @@  GstLibcameraAllocator *
 gst_libcamera_allocator_new(std::shared_ptr<Camera> camera,
 			    CameraConfiguration *config_)
 {
-	auto *self = GST_LIBCAMERA_ALLOCATOR(g_object_new(GST_TYPE_LIBCAMERA_ALLOCATOR,
-							  nullptr));
+	g_autoptr(GstLibcameraAllocator) self = GST_LIBCAMERA_ALLOCATOR(g_object_new(GST_TYPE_LIBCAMERA_ALLOCATOR,
+										     nullptr));
 	gint ret;

 	self->cm_ptr = new std::shared_ptr<CameraManager>(gst_libcamera_get_camera_manager(ret));
-	if (ret) {
-		g_object_unref(self);
+	if (ret)
 		return nullptr;
-	}

 	self->fb_allocator = new FrameBufferAllocator(camera);
 	for (StreamConfiguration &streamCfg : *config_) {
@@ -228,7 +228,7 @@  gst_libcamera_allocator_new(std::shared_ptr<Camera> camera,
 		g_hash_table_insert(self->pools, stream, pool);
 	}

-	return self;
+	return std::exchange(self, nullptr);
 }

 bool