[libcamera-devel] [PATCH v2 1/2] test: ipc: unixsocket: Close open fds on error paths

[Laurent Pinchart]

Hi Umang,

On Thu, Jun 04, 2020 at 10:53:16AM +0100, Kieran Bingham wrote:
> On 15/04/2020 16:04, Umang Jain wrote:
> > Pointed out by Coverity DefectId=279052
> > 
> > Signed-off-by: Umang Jain <email at uajain.com>
> > ---
> >  test/ipc/unixsocket.cpp | 19 ++++++++++++-------
> >  1 file changed, 12 insertions(+), 7 deletions(-)
> > 
> > diff --git a/test/ipc/unixsocket.cpp b/test/ipc/unixsocket.cpp
> > index 5348f35..7bc6a89 100644
> > --- a/test/ipc/unixsocket.cpp
> > +++ b/test/ipc/unixsocket.cpp
> > @@ -16,6 +16,7 @@
> >  #include <unistd.h>
> >  
> >  #include <libcamera/event_dispatcher.h>
> > +#include <libcamera/file_descriptor.h>
> >  #include <libcamera/timer.h>
> >  
> >  #include "ipc_unixsocket.h"
> > @@ -467,18 +468,22 @@ private:
> >  		if (fd < 0)
> >  			return fd;
> >  
> > +		FileDescriptor file_desc = FileDescriptor(fd);
> > +		close(fd);
> 
> Since you created this patch, the FileDescriptor has been updated to
> take move semantics to avoid having to close an FD directly after
> instantiating.
> 
> I think updating this patch to use that new API needs to happen in this
> patch, could you update please?
> 
> I'm not 100% sure of the syntax, but I think it's something like as
> simple as:
> 
> file_desc = FileDescriptor(std::move(fd));

This creates temporary file descriptor object and then uses copy
assignment. The compiler will optimize it out, but it's best to write

		FileDescriptor file_desc{ std::move(fd) };

> > +		if (file_desc.fd() < 0)
> > +			return file_desc.fd();
> > +
> >  		int size = 0;
> >  		for (unsigned int i = 0; i < num; i++) {
> > -			int clone = dup(fd);
> > -			if (clone < 0)
> > -				return clone;
> > +			FileDescriptor *clone = new FileDescriptor(file_desc.fd());
> > +			int clone_int = clone->fd();
> 
> I wouldn't use a local var here,, just reference clone->fd() below instead.

I wouldn't use FileDescriptor here actually. The reason why new is used
above is (I assume) to avoid closing the clone when the FileDescriptor
instance is destroyed, but that ends up leaking that new FileDescriptor
instance.

FileDescriptor is fine for file_desc (which should be named fileDesc as
we use camelCase), but here you can just keep the int clone variable.

> > +			if (clone_int < 0)
> > +				return clone_int;
> >  
> > -			size += calculateLength(clone);
> > -			message->fds.push_back(clone);
> > +			size += calculateLength(clone_int);
> > +			message->fds.push_back(clone_int);
> >  		}
> >  
> > -		close(fd);
> > -
> >  		return size;
> >  	}
> >  

-- 
Regards,

Laurent Pinchart