[libcamera-devel] libcamera: base: shared_fd: Don't dup() an invalid fd
diff mbox series

Message ID 20211206140701.19197-1-laurent.pinchart@ideasonboard.com
State Accepted
Commit 72679c682eb0b9f03948f258a26f78c037d33c48
Delegated to: Laurent Pinchart
Headers show
Series
  • [libcamera-devel] libcamera: base: shared_fd: Don't dup() an invalid fd
Related show

Commit Message

Laurent Pinchart Dec. 6, 2021, 2:07 p.m. UTC 
The SharedFD::dup() implementation calls the C library dup() function on
the fd. When the SharedFD instance is invalid, this produces an invalid
fd, which is the correct behaviour, but logs an error message.

Fix it by returning an invalid UniqueFD directly when the SharedFD is
invalid. This also saves a system call, which is always nice to do.

Fixes: fcf98514cb4e ("libcamera: base: file_descriptor: Return UniqueFD from dup()")
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/base/shared_fd.cpp | 3 +++
 1 file changed, 3 insertions(+)


base-commit: f8d2f17a3db5ac490d7b505253a98d6795c2b630

Comments

Kieran Bingham Dec. 6, 2021, 3:26 p.m. UTC | #1 
Quoting Laurent Pinchart (2021-12-06 14: 07:01)
> The SharedFD: :dup() implementation calls the C library dup() function on
Date: Mon, 06 Dec 2021 15:26:27 +0000
Message-ID: <163880438747.995700.268589314019276531@Monstersaurus>
User-Agent: alot/0.10

> the fd. When the SharedFD instance is invalid, this produces an invalid
> fd, which is the correct behaviour, but logs an error message.

Isn't logging an error in that event a reasonable thing to do?

> Fix it by returning an invalid UniqueFD directly when the SharedFD is
> invalid. This also saves a system call, which is always nice to do.

Saving that seems worthwhile anyway as we know it won't succeed.

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

> Fixes: fcf98514cb4e ("libcamera: base: file_descriptor: Return UniqueFD from dup()")
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/libcamera/base/shared_fd.cpp | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/src/libcamera/base/shared_fd.cpp b/src/libcamera/base/shared_fd.cpp
> index bd2ab5aa25b7..c711cf579a04 100644
> --- a/src/libcamera/base/shared_fd.cpp
> +++ b/src/libcamera/base/shared_fd.cpp
> @@ -253,6 +253,9 @@ SharedFD &SharedFD::operator=(SharedFD &&other)
>   */
>  UniqueFD SharedFD::dup() const
>  {
> +       if (!isValid())
> +               return {};
> +
>         UniqueFD dupFd(::dup(get()));
>         if (!dupFd.isValid()) {
>                 int ret = -errno;
> 
> base-commit: f8d2f17a3db5ac490d7b505253a98d6795c2b630
> -- 
> Regards,
> 
> Laurent Pinchart
>
Laurent Pinchart Dec. 6, 2021, 3:30 p.m. UTC | #2 
On Mon, Dec 06, 2021 at 03:26:30PM +0000, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2021-12-06 14: 07:01)
> > The SharedFD: :dup() implementation calls the C library dup() function on
> Date: Mon, 06 Dec 2021 15:26:27 +0000
> Message-ID: <163880438747.995700.268589314019276531@Monstersaurus>
> User-Agent: alot/0.10
> 
> > the fd. When the SharedFD instance is invalid, this produces an invalid
> > fd, which is the correct behaviour, but logs an error message.
> 
> Isn't logging an error in that event a reasonable thing to do?

There could be valid use cases for dup() on an invalid SharedFD, I'm not
sure.

> > Fix it by returning an invalid UniqueFD directly when the SharedFD is
> > invalid. This also saves a system call, which is always nice to do.
> 
> Saving that seems worthwhile anyway as we know it won't succeed.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > Fixes: fcf98514cb4e ("libcamera: base: file_descriptor: Return UniqueFD from dup()")
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  src/libcamera/base/shared_fd.cpp | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/src/libcamera/base/shared_fd.cpp b/src/libcamera/base/shared_fd.cpp
> > index bd2ab5aa25b7..c711cf579a04 100644
> > --- a/src/libcamera/base/shared_fd.cpp
> > +++ b/src/libcamera/base/shared_fd.cpp
> > @@ -253,6 +253,9 @@ SharedFD &SharedFD::operator=(SharedFD &&other)
> >   */
> >  UniqueFD SharedFD::dup() const
> >  {
> > +       if (!isValid())
> > +               return {};
> > +
> >         UniqueFD dupFd(::dup(get()));
> >         if (!dupFd.isValid()) {
> >                 int ret = -errno;
> > 
> > base-commit: f8d2f17a3db5ac490d7b505253a98d6795c2b630
Umang Jain Dec. 7, 2021, 4:37 p.m. UTC | #3 
Hi,

On 12/6/21 7:37 PM, Laurent Pinchart wrote:
> The SharedFD::dup() implementation calls the C library dup() function on
> the fd. When the SharedFD instance is invalid, this produces an invalid
> fd, which is the correct behaviour, but logs an error message.
>
> Fix it by returning an invalid UniqueFD directly when the SharedFD is
> invalid. This also saves a system call, which is always nice to do.
>
> Fixes: fcf98514cb4e ("libcamera: base: file_descriptor: Return UniqueFD from dup()")
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>   src/libcamera/base/shared_fd.cpp | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/src/libcamera/base/shared_fd.cpp b/src/libcamera/base/shared_fd.cpp
> index bd2ab5aa25b7..c711cf579a04 100644
> --- a/src/libcamera/base/shared_fd.cpp
> +++ b/src/libcamera/base/shared_fd.cpp
> @@ -253,6 +253,9 @@ SharedFD &SharedFD::operator=(SharedFD &&other)
>    */
>   UniqueFD SharedFD::dup() const
>   {
> +	if (!isValid())
> +		return {};
> +


simple enough

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

>   	UniqueFD dupFd(::dup(get()));
>   	if (!dupFd.isValid()) {
>   		int ret = -errno;
>
> base-commit: f8d2f17a3db5ac490d7b505253a98d6795c2b630

Patch
diff mbox series 

diff --git a/src/libcamera/base/shared_fd.cpp b/src/libcamera/base/shared_fd.cpp
index bd2ab5aa25b7..c711cf579a04 100644
--- a/src/libcamera/base/shared_fd.cpp
+++ b/src/libcamera/base/shared_fd.cpp
@@ -253,6 +253,9 @@  SharedFD &SharedFD::operator=(SharedFD &&other)
  */
 UniqueFD SharedFD::dup() const
 {
+	if (!isValid())
+		return {};
+
 	UniqueFD dupFd(::dup(get()));
 	if (!dupFd.isValid()) {
 		int ret = -errno;