[v1] libcamera: base: {Unique,Shared}FD: Warn if closing fails
diff mbox series

Message ID 20251023145906.1088030-1-barnabas.pocze@ideasonboard.com
State New
Headers show
Series
  • [v1] libcamera: base: {Unique,Shared}FD: Warn if closing fails
Related show

Commit Message

Barnabás Pőcze Oct. 23, 2025, 2:59 p.m. UTC 
If the contained file descriptor cannot be successfully closed,
that is usually a sign of a more serious invariant violation,
which deserves attention, so report those cases.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 src/libcamera/base/shared_fd.cpp | 7 +++++--
 src/libcamera/base/unique_fd.cpp | 8 ++++++--
 2 files changed, 11 insertions(+), 4 deletions(-)

Comments

Kieran Bingham Oct. 23, 2025, 3:05 p.m. UTC | #1 
Quoting Barnabás Pőcze (2025-10-23 15:59:06)
> If the contained file descriptor cannot be successfully closed,
> that is usually a sign of a more serious invariant violation,
> which deserves attention, so report those cases.

Ohh has this happened for you?

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

> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  src/libcamera/base/shared_fd.cpp | 7 +++++--
>  src/libcamera/base/unique_fd.cpp | 8 ++++++--
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/src/libcamera/base/shared_fd.cpp b/src/libcamera/base/shared_fd.cpp
> index 7afc8ca59..81b703a89 100644
> --- a/src/libcamera/base/shared_fd.cpp
> +++ b/src/libcamera/base/shared_fd.cpp
> @@ -284,8 +284,11 @@ SharedFD::Descriptor::Descriptor(int fd, bool duplicate)
>  
>  SharedFD::Descriptor::~Descriptor()
>  {
> -       if (fd_ != -1)
> -               close(fd_);
> +       if (fd_ >= 0 && close(fd_) < 0) {
> +               int ret = -errno;
> +               LOG(SharedFD, Error) << "Failed to close file descriptor "
> +                                    << fd_ << ": " << strerror(-ret);
> +       }
>  }
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/base/unique_fd.cpp b/src/libcamera/base/unique_fd.cpp
> index d0649e4de..178c08c4b 100644
> --- a/src/libcamera/base/unique_fd.cpp
> +++ b/src/libcamera/base/unique_fd.cpp
> @@ -7,6 +7,7 @@
>  
>  #include <libcamera/base/unique_fd.h>
>  
> +#include <string.h>
>  #include <unistd.h>
>  #include <utility>
>  
> @@ -98,8 +99,11 @@ void UniqueFD::reset(int fd)
>  
>         std::swap(fd, fd_);
>  
> -       if (fd >= 0)
> -               close(fd);
> +       if (fd >= 0 && close(fd) < 0) {
> +               int ret = -errno;
> +               LOG(UniqueFD, Error) << "Failed to close file descriptor "
> +                                    << fd << ": " << strerror(-ret);
> +       }
>  }
>  
>  /**
> -- 
> 2.51.1.dirty
>
Barnabás Pőcze Oct. 23, 2025, 3:14 p.m. UTC | #2 
2025. 10. 23. 17:05 keltezéssel, Kieran Bingham írta:
> Quoting Barnabás Pőcze (2025-10-23 15:59:06)
>> If the contained file descriptor cannot be successfully closed,
>> that is usually a sign of a more serious invariant violation,
>> which deserves attention, so report those cases.
> 
> Ohh has this happened for you?

There was an incorrect usage in pipewire that would've triggered this.

https://gitlab.freedesktop.org/pipewire/pipewire/-/commit/429c0e03a3cb49631638627e4097119fd26905f8


> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> ---
>>   src/libcamera/base/shared_fd.cpp | 7 +++++--
>>   src/libcamera/base/unique_fd.cpp | 8 ++++++--
>>   2 files changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/libcamera/base/shared_fd.cpp b/src/libcamera/base/shared_fd.cpp
>> index 7afc8ca59..81b703a89 100644
>> --- a/src/libcamera/base/shared_fd.cpp
>> +++ b/src/libcamera/base/shared_fd.cpp
>> @@ -284,8 +284,11 @@ SharedFD::Descriptor::Descriptor(int fd, bool duplicate)
>>   
>>   SharedFD::Descriptor::~Descriptor()
>>   {
>> -       if (fd_ != -1)
>> -               close(fd_);
>> +       if (fd_ >= 0 && close(fd_) < 0) {
>> +               int ret = -errno;
>> +               LOG(SharedFD, Error) << "Failed to close file descriptor "
>> +                                    << fd_ << ": " << strerror(-ret);
>> +       }
>>   }
>>   
>>   } /* namespace libcamera */
>> diff --git a/src/libcamera/base/unique_fd.cpp b/src/libcamera/base/unique_fd.cpp
>> index d0649e4de..178c08c4b 100644
>> --- a/src/libcamera/base/unique_fd.cpp
>> +++ b/src/libcamera/base/unique_fd.cpp
>> @@ -7,6 +7,7 @@
>>   
>>   #include <libcamera/base/unique_fd.h>
>>   
>> +#include <string.h>
>>   #include <unistd.h>
>>   #include <utility>
>>   
>> @@ -98,8 +99,11 @@ void UniqueFD::reset(int fd)
>>   
>>          std::swap(fd, fd_);
>>   
>> -       if (fd >= 0)
>> -               close(fd);
>> +       if (fd >= 0 && close(fd) < 0) {
>> +               int ret = -errno;
>> +               LOG(UniqueFD, Error) << "Failed to close file descriptor "
>> +                                    << fd << ": " << strerror(-ret);
>> +       }
>>   }
>>   
>>   /**
>> -- 
>> 2.51.1.dirty
>>
Jacopo Mondi Oct. 24, 2025, 7:38 a.m. UTC | #3 
Hi Barnabás

On Thu, Oct 23, 2025 at 04:59:06PM +0200, Barnabás Pőcze wrote:
> If the contained file descriptor cannot be successfully closed,
> that is usually a sign of a more serious invariant violation,
> which deserves attention, so report those cases.
>
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

> ---
>  src/libcamera/base/shared_fd.cpp | 7 +++++--
>  src/libcamera/base/unique_fd.cpp | 8 ++++++--
>  2 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/src/libcamera/base/shared_fd.cpp b/src/libcamera/base/shared_fd.cpp
> index 7afc8ca59..81b703a89 100644
> --- a/src/libcamera/base/shared_fd.cpp
> +++ b/src/libcamera/base/shared_fd.cpp
> @@ -284,8 +284,11 @@ SharedFD::Descriptor::Descriptor(int fd, bool duplicate)
>
>  SharedFD::Descriptor::~Descriptor()
>  {
> -	if (fd_ != -1)
> -		close(fd_);
> +	if (fd_ >= 0 && close(fd_) < 0) {
> +		int ret = -errno;
> +		LOG(SharedFD, Error) << "Failed to close file descriptor "
> +				     << fd_ << ": " << strerror(-ret);
> +	}
>  }
>
>  } /* namespace libcamera */
> diff --git a/src/libcamera/base/unique_fd.cpp b/src/libcamera/base/unique_fd.cpp
> index d0649e4de..178c08c4b 100644
> --- a/src/libcamera/base/unique_fd.cpp
> +++ b/src/libcamera/base/unique_fd.cpp
> @@ -7,6 +7,7 @@
>
>  #include <libcamera/base/unique_fd.h>
>
> +#include <string.h>
>  #include <unistd.h>
>  #include <utility>
>
> @@ -98,8 +99,11 @@ void UniqueFD::reset(int fd)
>
>  	std::swap(fd, fd_);
>
> -	if (fd >= 0)
> -		close(fd);
> +	if (fd >= 0 && close(fd) < 0) {
> +		int ret = -errno;
> +		LOG(UniqueFD, Error) << "Failed to close file descriptor "
> +				     << fd << ": " << strerror(-ret);
> +	}
>  }
>
>  /**
> --
> 2.51.1.dirty
>
Kieran Bingham Oct. 24, 2025, 8:45 a.m. UTC | #4 
Quoting Barnabás Pőcze (2025-10-23 16:14:09)
> 2025. 10. 23. 17:05 keltezéssel, Kieran Bingham írta:
> > Quoting Barnabás Pőcze (2025-10-23 15:59:06)
> >> If the contained file descriptor cannot be successfully closed,
> >> that is usually a sign of a more serious invariant violation,
> >> which deserves attention, so report those cases.
> > 
> > Ohh has this happened for you?
> 
> There was an incorrect usage in pipewire that would've triggered this.
> 
> https://gitlab.freedesktop.org/pipewire/pipewire/-/commit/429c0e03a3cb49631638627e4097119fd26905f8
> 

I see. And we /want/ the applications to have the fd - and we can't stop
them closing it - so indeed, this looks like a good protection to add.

Thanks.


> 
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > 
> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> >> ---
> >>   src/libcamera/base/shared_fd.cpp | 7 +++++--
> >>   src/libcamera/base/unique_fd.cpp | 8 ++++++--
> >>   2 files changed, 11 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/src/libcamera/base/shared_fd.cpp b/src/libcamera/base/shared_fd.cpp
> >> index 7afc8ca59..81b703a89 100644
> >> --- a/src/libcamera/base/shared_fd.cpp
> >> +++ b/src/libcamera/base/shared_fd.cpp
> >> @@ -284,8 +284,11 @@ SharedFD::Descriptor::Descriptor(int fd, bool duplicate)
> >>   
> >>   SharedFD::Descriptor::~Descriptor()
> >>   {
> >> -       if (fd_ != -1)
> >> -               close(fd_);
> >> +       if (fd_ >= 0 && close(fd_) < 0) {
> >> +               int ret = -errno;
> >> +               LOG(SharedFD, Error) << "Failed to close file descriptor "
> >> +                                    << fd_ << ": " << strerror(-ret);
> >> +       }
> >>   }
> >>   
> >>   } /* namespace libcamera */
> >> diff --git a/src/libcamera/base/unique_fd.cpp b/src/libcamera/base/unique_fd.cpp
> >> index d0649e4de..178c08c4b 100644
> >> --- a/src/libcamera/base/unique_fd.cpp
> >> +++ b/src/libcamera/base/unique_fd.cpp
> >> @@ -7,6 +7,7 @@
> >>   
> >>   #include <libcamera/base/unique_fd.h>
> >>   
> >> +#include <string.h>
> >>   #include <unistd.h>
> >>   #include <utility>
> >>   
> >> @@ -98,8 +99,11 @@ void UniqueFD::reset(int fd)
> >>   
> >>          std::swap(fd, fd_);
> >>   
> >> -       if (fd >= 0)
> >> -               close(fd);
> >> +       if (fd >= 0 && close(fd) < 0) {
> >> +               int ret = -errno;
> >> +               LOG(UniqueFD, Error) << "Failed to close file descriptor "
> >> +                                    << fd << ": " << strerror(-ret);
> >> +       }
> >>   }
> >>   
> >>   /**
> >> -- 
> >> 2.51.1.dirty
> >>
>

Patch
diff mbox series 

diff --git a/src/libcamera/base/shared_fd.cpp b/src/libcamera/base/shared_fd.cpp
index 7afc8ca59..81b703a89 100644
--- a/src/libcamera/base/shared_fd.cpp
+++ b/src/libcamera/base/shared_fd.cpp
@@ -284,8 +284,11 @@  SharedFD::Descriptor::Descriptor(int fd, bool duplicate)
 
 SharedFD::Descriptor::~Descriptor()
 {
-	if (fd_ != -1)
-		close(fd_);
+	if (fd_ >= 0 && close(fd_) < 0) {
+		int ret = -errno;
+		LOG(SharedFD, Error) << "Failed to close file descriptor "
+				     << fd_ << ": " << strerror(-ret);
+	}
 }
 
 } /* namespace libcamera */
diff --git a/src/libcamera/base/unique_fd.cpp b/src/libcamera/base/unique_fd.cpp
index d0649e4de..178c08c4b 100644
--- a/src/libcamera/base/unique_fd.cpp
+++ b/src/libcamera/base/unique_fd.cpp
@@ -7,6 +7,7 @@ 
 
 #include <libcamera/base/unique_fd.h>
 
+#include <string.h>
 #include <unistd.h>
 #include <utility>
 
@@ -98,8 +99,11 @@  void UniqueFD::reset(int fd)
 
 	std::swap(fd, fd_);
 
-	if (fd >= 0)
-		close(fd);
+	if (fd >= 0 && close(fd) < 0) {
+		int ret = -errno;
+		LOG(UniqueFD, Error) << "Failed to close file descriptor "
+				     << fd << ": " << strerror(-ret);
+	}
 }
 
 /**