[libcamera-devel] [RFC PATCH] ipa: IPADataSerializer: Fix FileDescriptor deserialization

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Tue, Jul 20, 2021 at 07:28:53PM +0900, Paul Elder wrote:
> Previously deserializing a FileDescriptor would use the copy
> constructor, causing an fd leak. Fix this by copying the fd integer to
> force the FileDescriptor to use the move constructor.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> 
> ---
> Umang, could you please check if this fixes the issue?
> 
> I'll remove the error message; it's just there for debugging (or we can
> keep it to protect against unforseen errors?)
> ---
>  src/libcamera/ipa_data_serializer.cpp | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/ipa_data_serializer.cpp b/src/libcamera/ipa_data_serializer.cpp
> index fb941e6b..2a527d5d 100644
> --- a/src/libcamera/ipa_data_serializer.cpp
> +++ b/src/libcamera/ipa_data_serializer.cpp
> @@ -8,6 +8,7 @@
>  #include "libcamera/internal/ipa_data_serializer.h"
>  
>  #include <libcamera/base/log.h>
> +#include <unistd.h>
>  
>  /**
>   * \file ipa_data_serializer.h
> @@ -547,7 +548,14 @@ FileDescriptor IPADataSerializer<FileDescriptor>::deserialize(std::vector<uint8_
>  
>  	ASSERT(!(valid && std::distance(fdsBegin, fdsEnd) < 1));
>  
> -	return valid ? FileDescriptor(*fdsBegin) : FileDescriptor();
> +	int tmpfd = valid ? *fdsBegin : -1;
> +	FileDescriptor fd = FileDescriptor(tmpfd);

Don't you need a std::move(tmpfd) here to use the move constructor ?

To be honest, I'm still feeling a bit uneasy about the large distance
between the fd creation (when we receive the data from the unix socket)
and the wrapping in a FileDescriptor. There's nothing that protects
against leaks between those two points. At the very least I think we
should turn the const iterator to a non-const iterator, and walk the
vector of file descriptors after we're done deserializing to ensure that
all entries are equal to -1, as a defensive measure, but then I'm not
sure why we shouldn't go all the way and use FileDescriptor in the IPC
message. The more we can use the language features to ensure that bugs
can't happen, the better.

> +	if (valid && tmpfd != -1) {
> +		LOG(IPADataSerializer, Error) << "We probably leaked an fd";
> +		close(tmpfd);
> +	}
> +
> +	return fd;
>  }
>  
>  template<>

-- 
Regards,

Laurent Pinchart