[libcamera-devel] [PATCH] test: threads: Fix memory leak

[Laurent Pinchart]

Hi Sebastian,

On Mon, Apr 12, 2021 at 06:21:36AM +0200, Sebastian Fricke wrote:
> Hey Laurent,
> 
> Thank you for the patch.
> 
> On 12.04.2021 00:36, Laurent Pinchart wrote:
> > The last instance of Thread allocated in the test is never deleted, not
> 
> s/not/nor/ ?

Oops, indeed.

> > are other instances deleted in error paths. Use a std::unique_ptr<> to
> > ensure deletion.
> 
> I tested this fix and valgrind does not report any leaked memory from
> the libcamera tests. But just out of curiosity do you get a similar
> report in valgrind:
> ```
> basti at nanopct4:~/libcamera$ valgrind --leak-check=full --show-leak-kinds=all ninja -C build test
> ...
> libcamera 0.0.0
> 
>    Configuration
>      Enabled pipelines     : ipu3
>                              raspberrypi
>                              rkisp1
>                              simple
>                              uvcvideo
>                              vimc
>      Android support       : False
>      GStreamer support     : False
>      V4L2 emulation support: False
>      cam application       : True
>      qcam application      : False
>      Unit tests            : True
> 
> Option werror is: true [default: true]
> ...
> ==6151== LEAK SUMMARY:
> ==6151==    definitely lost: 296 bytes in 1 blocks
> ==6151==    indirectly lost: 576 bytes in 2 blocks
> ==6151==      possibly lost: 0 bytes in 0 blocks
> ==6151==    still reachable: 2,725,231 bytes in 15,220 blocks
> ==6151==         suppressed: 0 bytes in 0 blocks
> ```
> 
> Those are all related to our build tool ninja.

That's checking for leaks in ninja itself, it's not ours.

> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> Tested-by: Sebastian Fricke <sebastian.fricke at posteo.net>
> 
> > ---
> >  test/threads.cpp | 19 ++++++++-----------
> >  1 file changed, 8 insertions(+), 11 deletions(-)
> > 
> > diff --git a/test/threads.cpp b/test/threads.cpp
> > index b4b8d913cd2b..e0c50dc90a65 100644
> > --- a/test/threads.cpp
> > +++ b/test/threads.cpp
> > @@ -7,6 +7,7 @@
> > 
> >  #include <chrono>
> >  #include <iostream>
> > +#include <memory>
> >  #include <thread>
> > 
> >  #include "libcamera/internal/thread.h"
> > @@ -45,23 +46,23 @@ protected:
> >  	int run()
> >  	{
> >  		/* Test Thread() retrieval for the main thread. */
> > -		Thread *thread = Thread::current();
> > -		if (!thread) {
> > +		Thread *mainThread = Thread::current();
> > +		if (!mainThread) {
> >  			cout << "Thread::current() failed to main thread"
> >  			     << endl;
> >  			return TestFail;
> >  		}
> > 
> > -		if (!thread->isRunning()) {
> > +		if (!mainThread->isRunning()) {
> >  			cout << "Main thread is not running" << endl;
> >  			return TestFail;
> >  		}
> > 
> >  		/* Test starting the main thread, the test shall not crash. */
> > -		thread->start();
> > +		mainThread->start();
> > 
> >  		/* Test the running state of a custom thread. */
> > -		thread = new Thread();
> > +		std::unique_ptr<Thread> thread = std::make_unique<Thread>();
> >  		thread->start();
> > 
> >  		if (!thread->isRunning()) {
> > @@ -79,10 +80,8 @@ protected:
> >  			return TestFail;
> >  		}
> > 
> > -		delete thread;
> > -
> >  		/* Test waiting for completion with a timeout. */
> > -		thread = new DelayThread(chrono::milliseconds(500));
> > +		thread = std::make_unique<DelayThread>(chrono::milliseconds(500));
> >  		thread->start();
> >  		thread->exit(0);
> > 
> > @@ -100,10 +99,8 @@ protected:
> >  			return TestFail;
> >  		}
> > 
> > -		delete thread;
> > -
> >  		/* Test waiting on a thread that isn't running. */
> > -		thread = new Thread();
> > +		thread = std::make_unique<Thread>();
> > 
> >  		timeout = !thread->wait();
> >  		if (timeout) {

-- 
Regards,

Laurent Pinchart