| Message ID | 20200514084003.16948-1-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Commit | 353fc4c223225f32176f9fc35a832d633486aa0d |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, On 14/05/2020 09:40, Naushir Patuck wrote: > The FileDescriptor constructor used in V4L2VideoDevice::exportDmabufFd() > creates a duplicate of the fd to store in the object. The original > fd returned by the VIDIOC_EXPBUF ioctl was never closed, and left > dangling. This would cause out of memory conditions if the camera stream > was repeatedly started and stopped. > > This change closes the original fd explicitly, fixing the leak. > Ouch. And agreed, Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/libcamera/v4l2_videodevice.cpp | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index 4b9f8b5c..2f9c1333 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -1276,7 +1276,14 @@ FileDescriptor V4L2VideoDevice::exportDmabufFd(unsigned int index, > return FileDescriptor(); > } > > - return FileDescriptor(expbuf.fd); > + FileDescriptor fd(expbuf.fd); > + /* > + * FileDescriptor takes a duplicate of fd, so we must close the > + * original here, otherwise it will be left dangling. > + */ > + ::close(expbuf.fd); > + > + return fd; > } > > /** >
Hi Naush, Thank you for the patch. On Thu, May 14, 2020 at 09:40:03AM +0100, Naushir Patuck wrote: > The FileDescriptor constructor used in V4L2VideoDevice::exportDmabufFd() > creates a duplicate of the fd to store in the object. The original > fd returned by the VIDIOC_EXPBUF ioctl was never closed, and left > dangling. This would cause out of memory conditions if the camera stream > was repeatedly started and stopped. > > This change closes the original fd explicitly, fixing the leak. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> and applied. This issue however shows that the FileDescriptor class API is not ideal. I'll propose enhancements and CC you. > --- > src/libcamera/v4l2_videodevice.cpp | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index 4b9f8b5c..2f9c1333 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -1276,7 +1276,14 @@ FileDescriptor V4L2VideoDevice::exportDmabufFd(unsigned int index, > return FileDescriptor(); > } > > - return FileDescriptor(expbuf.fd); > + FileDescriptor fd(expbuf.fd); > + /* > + * FileDescriptor takes a duplicate of fd, so we must close the > + * original here, otherwise it will be left dangling. > + */ > + ::close(expbuf.fd); > + > + return fd; > } > > /**
diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp index 4b9f8b5c..2f9c1333 100644 --- a/src/libcamera/v4l2_videodevice.cpp +++ b/src/libcamera/v4l2_videodevice.cpp @@ -1276,7 +1276,14 @@ FileDescriptor V4L2VideoDevice::exportDmabufFd(unsigned int index, return FileDescriptor(); } - return FileDescriptor(expbuf.fd); + FileDescriptor fd(expbuf.fd); + /* + * FileDescriptor takes a duplicate of fd, so we must close the + * original here, otherwise it will be left dangling. + */ + ::close(expbuf.fd); + + return fd; } /**
The FileDescriptor constructor used in V4L2VideoDevice::exportDmabufFd() creates a duplicate of the fd to store in the object. The original fd returned by the VIDIOC_EXPBUF ioctl was never closed, and left dangling. This would cause out of memory conditions if the camera stream was repeatedly started and stopped. This change closes the original fd explicitly, fixing the leak. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/libcamera/v4l2_videodevice.cpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)