[libcamera-devel,RFC,10/10] v4l2: V4L2Camera: Return int in getBufferFd()
diff mbox series

Message ID 20210415083843.3399502-10-hiroh@chromium.org
State Changes Requested
Headers show
Series
  • Untitled series #1933
Related show

Commit Message

Hirokazu Honda April 15, 2021, 8:38 a.m. UTC 
V4L2Camera::getBufferFd() returns FileDescriptor. However, the
file descriptor is still owned by V4L2Camera. It should rather
return an integer to represent V4L2Camera doesn't have the
ownership of the file descriptor.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
---
 src/v4l2/v4l2_camera.cpp       | 6 +++---
 src/v4l2/v4l2_camera.h         | 2 +-
 src/v4l2/v4l2_camera_proxy.cpp | 6 +++---
 3 files changed, 7 insertions(+), 7 deletions(-)

Comments

Laurent Pinchart June 6, 2021, 6:56 p.m. UTC | #1 
Hi Hiro,

Thank you for the patch.

On Thu, Apr 15, 2021 at 05:38:43PM +0900, Hirokazu Honda wrote:
> V4L2Camera::getBufferFd() returns FileDescriptor. However, the
> file descriptor is still owned by V4L2Camera. It should rather
> return an integer to represent V4L2Camera doesn't have the
> ownership of the file descriptor.

The FileDescriptor class doesn't imply ownership, as it's similar to a
shared_ptr<>. I'm not against this patch, but why do you think
FileDescriptor is bad here ?

> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> ---
>  src/v4l2/v4l2_camera.cpp       | 6 +++---
>  src/v4l2/v4l2_camera.h         | 2 +-
>  src/v4l2/v4l2_camera_proxy.cpp | 6 +++---
>  3 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> index 97825c71..cccc6749 100644
> --- a/src/v4l2/v4l2_camera.cpp
> +++ b/src/v4l2/v4l2_camera.cpp
> @@ -186,16 +186,16 @@ void V4L2Camera::freeBuffers()
>  	bufferAllocator_->free(stream);
>  }
>  
> -FileDescriptor V4L2Camera::getBufferFd(unsigned int index)
> +int V4L2Camera::getBufferFd(unsigned int index)
>  {
>  	Stream *stream = config_->at(0).stream();
>  	const std::vector<std::unique_ptr<FrameBuffer>> &buffers =
>  		bufferAllocator_->buffers(stream);
>  
>  	if (buffers.size() <= index)
> -		return FileDescriptor();
> +		return -1;
>  
> -	return buffers[index]->planes()[0].fd;
> +	return buffers[index]->planes()[0].fd.fd();
>  }
>  
>  int V4L2Camera::streamOn()
> diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h
> index d2380462..8efe1642 100644
> --- a/src/v4l2/v4l2_camera.h
> +++ b/src/v4l2/v4l2_camera.h
> @@ -53,7 +53,7 @@ public:
>  
>  	int allocBuffers(unsigned int count);
>  	void freeBuffers();
> -	FileDescriptor getBufferFd(unsigned int index);
> +	int getBufferFd(unsigned int index);
>  
>  	int streamOn();
>  	int streamOff();
> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> index f8bfe595..8f417421 100644
> --- a/src/v4l2/v4l2_camera_proxy.cpp
> +++ b/src/v4l2/v4l2_camera_proxy.cpp
> @@ -112,14 +112,14 @@ void *V4L2CameraProxy::mmap(void *addr, size_t length, int prot, int flags,
>  		return MAP_FAILED;
>  	}
>  
> -	FileDescriptor fd = vcam_->getBufferFd(index);
> -	if (!fd.isValid()) {
> +	int fd = vcam_->getBufferFd(index);
> +	if (fd < 0) {
>  		errno = EINVAL;
>  		return MAP_FAILED;
>  	}
>  
>  	void *map = V4L2CompatManager::instance()->fops().mmap(addr, length, prot,
> -							       flags, fd.fd(), 0);
> +							       flags, fd, 0);
>  	if (map == MAP_FAILED)
>  		return map;
>
Hirokazu Honda June 7, 2021, 9:22 a.m. UTC | #2 
Hi Laurent,

On Mon, Jun 7, 2021 at 3:56 AM Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> Hi Hiro,
>
> Thank you for the patch.
>
> On Thu, Apr 15, 2021 at 05:38:43PM +0900, Hirokazu Honda wrote:
> > V4L2Camera::getBufferFd() returns FileDescriptor. However, the
> > file descriptor is still owned by V4L2Camera. It should rather
> > return an integer to represent V4L2Camera doesn't have the
> > ownership of the file descriptor.
>
> The FileDescriptor class doesn't imply ownership, as it's similar to a
> shared_ptr<>. I'm not against this patch, but why do you think
> FileDescriptor is bad here ?
>
>
IMO, returning FileDescriptor implies that a caller shares the ownership of
it.
It enables a caller to keep it outlive a callee and even share its
ownership with others.
What do you think?

-Hiro

> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > ---
> >  src/v4l2/v4l2_camera.cpp       | 6 +++---
> >  src/v4l2/v4l2_camera.h         | 2 +-
> >  src/v4l2/v4l2_camera_proxy.cpp | 6 +++---
> >  3 files changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> > index 97825c71..cccc6749 100644
> > --- a/src/v4l2/v4l2_camera.cpp
> > +++ b/src/v4l2/v4l2_camera.cpp
> > @@ -186,16 +186,16 @@ void V4L2Camera::freeBuffers()
> >       bufferAllocator_->free(stream);
> >  }
> >
> > -FileDescriptor V4L2Camera::getBufferFd(unsigned int index)
> > +int V4L2Camera::getBufferFd(unsigned int index)
> >  {
> >       Stream *stream = config_->at(0).stream();
> >       const std::vector<std::unique_ptr<FrameBuffer>> &buffers =
> >               bufferAllocator_->buffers(stream);
> >
> >       if (buffers.size() <= index)
> > -             return FileDescriptor();
> > +             return -1;
> >
> > -     return buffers[index]->planes()[0].fd;
> > +     return buffers[index]->planes()[0].fd.fd();
> >  }
> >
> >  int V4L2Camera::streamOn()
> > diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h
> > index d2380462..8efe1642 100644
> > --- a/src/v4l2/v4l2_camera.h
> > +++ b/src/v4l2/v4l2_camera.h
> > @@ -53,7 +53,7 @@ public:
> >
> >       int allocBuffers(unsigned int count);
> >       void freeBuffers();
> > -     FileDescriptor getBufferFd(unsigned int index);
> > +     int getBufferFd(unsigned int index);
> >
> >       int streamOn();
> >       int streamOff();
> > diff --git a/src/v4l2/v4l2_camera_proxy.cpp
> b/src/v4l2/v4l2_camera_proxy.cpp
> > index f8bfe595..8f417421 100644
> > --- a/src/v4l2/v4l2_camera_proxy.cpp
> > +++ b/src/v4l2/v4l2_camera_proxy.cpp
> > @@ -112,14 +112,14 @@ void *V4L2CameraProxy::mmap(void *addr, size_t
> length, int prot, int flags,
> >               return MAP_FAILED;
> >       }
> >
> > -     FileDescriptor fd = vcam_->getBufferFd(index);
> > -     if (!fd.isValid()) {
> > +     int fd = vcam_->getBufferFd(index);
> > +     if (fd < 0) {
> >               errno = EINVAL;
> >               return MAP_FAILED;
> >       }
> >
> >       void *map = V4L2CompatManager::instance()->fops().mmap(addr,
> length, prot,
> > -                                                            flags,
> fd.fd(), 0);
> > +                                                            flags, fd,
> 0);
> >       if (map == MAP_FAILED)
> >               return map;
> >
>
> --
> Regards,
>
> Laurent Pinchart
>
Laurent Pinchart June 28, 2021, 3:03 a.m. UTC | #3 
Hi Hiro,

On Mon, Jun 07, 2021 at 06:22:21PM +0900, Hirokazu Honda wrote:
> On Mon, Jun 7, 2021 at 3:56 AM Laurent Pinchart wrote:
> > On Thu, Apr 15, 2021 at 05:38:43PM +0900, Hirokazu Honda wrote:
> > > V4L2Camera::getBufferFd() returns FileDescriptor. However, the
> > > file descriptor is still owned by V4L2Camera. It should rather
> > > return an integer to represent V4L2Camera doesn't have the
> > > ownership of the file descriptor.
> >
> > The FileDescriptor class doesn't imply ownership, as it's similar to a
> > shared_ptr<>. I'm not against this patch, but why do you think
> > FileDescriptor is bad here ?
>
> IMO, returning FileDescriptor implies that a caller shares the ownership of it.
> It enables a caller to keep it outlive a callee and even share its
> ownership with others.
> What do you think?

Generally speaking, it may be dangerous to handle file descriptors as
integers, due not only to the risk of leakage, but also to the risk of
use-after-free (or rather use-after-close in this case). FileDescriptor
prevents that.

On the other hand, I agree with you that in this case the caller isn't
meant to use the file descriptor in any way that would outlive the
descriptor stored in V4L2Camera. If it was a generic API, I'd prefer
keeping FileDescriptor, but as V4L2Camera and V4L2CameraProxy are
tightly coupled, it really doesn't matter much. I'm OK with the patch if
you think it's better.

> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > > ---
> > >  src/v4l2/v4l2_camera.cpp       | 6 +++---
> > >  src/v4l2/v4l2_camera.h         | 2 +-
> > >  src/v4l2/v4l2_camera_proxy.cpp | 6 +++---
> > >  3 files changed, 7 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> > > index 97825c71..cccc6749 100644
> > > --- a/src/v4l2/v4l2_camera.cpp
> > > +++ b/src/v4l2/v4l2_camera.cpp
> > > @@ -186,16 +186,16 @@ void V4L2Camera::freeBuffers()
> > >       bufferAllocator_->free(stream);
> > >  }
> > >
> > > -FileDescriptor V4L2Camera::getBufferFd(unsigned int index)
> > > +int V4L2Camera::getBufferFd(unsigned int index)
> > >  {
> > >       Stream *stream = config_->at(0).stream();
> > >       const std::vector<std::unique_ptr<FrameBuffer>> &buffers =
> > >               bufferAllocator_->buffers(stream);
> > >
> > >       if (buffers.size() <= index)
> > > -             return FileDescriptor();
> > > +             return -1;
> > >
> > > -     return buffers[index]->planes()[0].fd;
> > > +     return buffers[index]->planes()[0].fd.fd();
> > >  }
> > >
> > >  int V4L2Camera::streamOn()
> > > diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h
> > > index d2380462..8efe1642 100644
> > > --- a/src/v4l2/v4l2_camera.h
> > > +++ b/src/v4l2/v4l2_camera.h
> > > @@ -53,7 +53,7 @@ public:
> > >
> > >       int allocBuffers(unsigned int count);
> > >       void freeBuffers();
> > > -     FileDescriptor getBufferFd(unsigned int index);
> > > +     int getBufferFd(unsigned int index);
> > >
> > >       int streamOn();
> > >       int streamOff();
> > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> > > index f8bfe595..8f417421 100644
> > > --- a/src/v4l2/v4l2_camera_proxy.cpp
> > > +++ b/src/v4l2/v4l2_camera_proxy.cpp
> > > @@ -112,14 +112,14 @@ void *V4L2CameraProxy::mmap(void *addr, size_t length, int prot, int flags,
> > >               return MAP_FAILED;
> > >       }
> > >
> > > -     FileDescriptor fd = vcam_->getBufferFd(index);
> > > -     if (!fd.isValid()) {
> > > +     int fd = vcam_->getBufferFd(index);
> > > +     if (fd < 0) {
> > >               errno = EINVAL;
> > >               return MAP_FAILED;
> > >       }
> > >
> > >       void *map = V4L2CompatManager::instance()->fops().mmap(addr, length, prot,
> > > -                                                            flags, fd.fd(), 0);
> > > +                                                            flags, fd, 0);
> > >       if (map == MAP_FAILED)
> > >               return map;
> > >
Hirokazu Honda June 28, 2021, 9:11 p.m. UTC | #4 
Hi Laurent,

On Mon, Jun 28, 2021 at 12:03 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Hiro,
>
> On Mon, Jun 07, 2021 at 06:22:21PM +0900, Hirokazu Honda wrote:
> > On Mon, Jun 7, 2021 at 3:56 AM Laurent Pinchart wrote:
> > > On Thu, Apr 15, 2021 at 05:38:43PM +0900, Hirokazu Honda wrote:
> > > > V4L2Camera::getBufferFd() returns FileDescriptor. However, the
> > > > file descriptor is still owned by V4L2Camera. It should rather
> > > > return an integer to represent V4L2Camera doesn't have the
> > > > ownership of the file descriptor.
> > >
> > > The FileDescriptor class doesn't imply ownership, as it's similar to a
> > > shared_ptr<>. I'm not against this patch, but why do you think
> > > FileDescriptor is bad here ?
> >
> > IMO, returning FileDescriptor implies that a caller shares the ownership of it.
> > It enables a caller to keep it outlive a callee and even share its
> > ownership with others.
> > What do you think?
>
> Generally speaking, it may be dangerous to handle file descriptors as
> integers, due not only to the risk of leakage, but also to the risk of
> use-after-free (or rather use-after-close in this case). FileDescriptor
> prevents that.
>
> On the other hand, I agree with you that in this case the caller isn't
> meant to use the file descriptor in any way that would outlive the
> descriptor stored in V4L2Camera. If it was a generic API, I'd prefer
> keeping FileDescriptor, but as V4L2Camera and V4L2CameraProxy are
> tightly coupled, it really doesn't matter much. I'm OK with the patch if
> you think it's better.
>

I would like to keep my idea as the current getBufferFd usage intends
to not share the ownership.
If the usage needs to be changed, I think it will be better to use
FileDescriptor at that time.

Thanks,
-Hiro
> > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > > > ---
> > > >  src/v4l2/v4l2_camera.cpp       | 6 +++---
> > > >  src/v4l2/v4l2_camera.h         | 2 +-
> > > >  src/v4l2/v4l2_camera_proxy.cpp | 6 +++---
> > > >  3 files changed, 7 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> > > > index 97825c71..cccc6749 100644
> > > > --- a/src/v4l2/v4l2_camera.cpp
> > > > +++ b/src/v4l2/v4l2_camera.cpp
> > > > @@ -186,16 +186,16 @@ void V4L2Camera::freeBuffers()
> > > >       bufferAllocator_->free(stream);
> > > >  }
> > > >
> > > > -FileDescriptor V4L2Camera::getBufferFd(unsigned int index)
> > > > +int V4L2Camera::getBufferFd(unsigned int index)
> > > >  {
> > > >       Stream *stream = config_->at(0).stream();
> > > >       const std::vector<std::unique_ptr<FrameBuffer>> &buffers =
> > > >               bufferAllocator_->buffers(stream);
> > > >
> > > >       if (buffers.size() <= index)
> > > > -             return FileDescriptor();
> > > > +             return -1;
> > > >
> > > > -     return buffers[index]->planes()[0].fd;
> > > > +     return buffers[index]->planes()[0].fd.fd();
> > > >  }
> > > >
> > > >  int V4L2Camera::streamOn()
> > > > diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h
> > > > index d2380462..8efe1642 100644
> > > > --- a/src/v4l2/v4l2_camera.h
> > > > +++ b/src/v4l2/v4l2_camera.h
> > > > @@ -53,7 +53,7 @@ public:
> > > >
> > > >       int allocBuffers(unsigned int count);
> > > >       void freeBuffers();
> > > > -     FileDescriptor getBufferFd(unsigned int index);
> > > > +     int getBufferFd(unsigned int index);
> > > >
> > > >       int streamOn();
> > > >       int streamOff();
> > > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> > > > index f8bfe595..8f417421 100644
> > > > --- a/src/v4l2/v4l2_camera_proxy.cpp
> > > > +++ b/src/v4l2/v4l2_camera_proxy.cpp
> > > > @@ -112,14 +112,14 @@ void *V4L2CameraProxy::mmap(void *addr, size_t length, int prot, int flags,
> > > >               return MAP_FAILED;
> > > >       }
> > > >
> > > > -     FileDescriptor fd = vcam_->getBufferFd(index);
> > > > -     if (!fd.isValid()) {
> > > > +     int fd = vcam_->getBufferFd(index);
> > > > +     if (fd < 0) {
> > > >               errno = EINVAL;
> > > >               return MAP_FAILED;
> > > >       }
> > > >
> > > >       void *map = V4L2CompatManager::instance()->fops().mmap(addr, length, prot,
> > > > -                                                            flags, fd.fd(), 0);
> > > > +                                                            flags, fd, 0);
> > > >       if (map == MAP_FAILED)
> > > >               return map;
> > > >
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
index 97825c71..cccc6749 100644
--- a/src/v4l2/v4l2_camera.cpp
+++ b/src/v4l2/v4l2_camera.cpp
@@ -186,16 +186,16 @@  void V4L2Camera::freeBuffers()
 	bufferAllocator_->free(stream);
 }
 
-FileDescriptor V4L2Camera::getBufferFd(unsigned int index)
+int V4L2Camera::getBufferFd(unsigned int index)
 {
 	Stream *stream = config_->at(0).stream();
 	const std::vector<std::unique_ptr<FrameBuffer>> &buffers =
 		bufferAllocator_->buffers(stream);
 
 	if (buffers.size() <= index)
-		return FileDescriptor();
+		return -1;
 
-	return buffers[index]->planes()[0].fd;
+	return buffers[index]->planes()[0].fd.fd();
 }
 
 int V4L2Camera::streamOn()
diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h
index d2380462..8efe1642 100644
--- a/src/v4l2/v4l2_camera.h
+++ b/src/v4l2/v4l2_camera.h
@@ -53,7 +53,7 @@  public:
 
 	int allocBuffers(unsigned int count);
 	void freeBuffers();
-	FileDescriptor getBufferFd(unsigned int index);
+	int getBufferFd(unsigned int index);
 
 	int streamOn();
 	int streamOff();
diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
index f8bfe595..8f417421 100644
--- a/src/v4l2/v4l2_camera_proxy.cpp
+++ b/src/v4l2/v4l2_camera_proxy.cpp
@@ -112,14 +112,14 @@  void *V4L2CameraProxy::mmap(void *addr, size_t length, int prot, int flags,
 		return MAP_FAILED;
 	}
 
-	FileDescriptor fd = vcam_->getBufferFd(index);
-	if (!fd.isValid()) {
+	int fd = vcam_->getBufferFd(index);
+	if (fd < 0) {
 		errno = EINVAL;
 		return MAP_FAILED;
 	}
 
 	void *map = V4L2CompatManager::instance()->fops().mmap(addr, length, prot,
-							       flags, fd.fd(), 0);
+							       flags, fd, 0);
 	if (map == MAP_FAILED)
 		return map;