Message ID | 20210720102853.28541-1-paul.elder@ideasonboard.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
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@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<>
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); + if (valid && tmpfd != -1) { + LOG(IPADataSerializer, Error) << "We probably leaked an fd"; + close(tmpfd); + } + + return fd; } template<>
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@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(-)