[libcamera-devel] [PATCH v2 03/17] v4l2: v4l2_camera_proxy: Check for null arg values in main ioctl handler
Jacopo Mondi
jacopo at jmondi.org
Fri Jun 19 12:22:30 CEST 2020
Hi Paul,
On Fri, Jun 19, 2020 at 02:41:09PM +0900, Paul Elder wrote:
> The ioctl handlers currently don't check if arg is null, so if it ever
> is, it will cause a segfault. Check that arg is null and return -EFAULT
> in the main vidioc ioctl handler.
>
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
>
> ---
> Changes in v2:
> - moved !arg check to main ioctl handler, and added a set of supported
> ioctls
> - use !arg instead of arg == nullptr
> ---
> src/v4l2/v4l2_camera_proxy.cpp | 27 +++++++++++++++++++++++++--
> src/v4l2/v4l2_camera_proxy.h | 3 +++
> 2 files changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> index f06f58d..cff6562 100644
> --- a/src/v4l2/v4l2_camera_proxy.cpp
> +++ b/src/v4l2/v4l2_camera_proxy.cpp
> @@ -11,6 +11,7 @@
> #include <array>
> #include <errno.h>
> #include <linux/videodev2.h>
> +#include <set>
> #include <string.h>
> #include <sys/mman.h>
> #include <unistd.h>
> @@ -238,7 +239,6 @@ int V4L2CameraProxy::vidioc_enum_fmt(V4L2CameraFile *cf, struct v4l2_fmtdesc *ar
> {
> LOG(V4L2Compat, Debug) << "Servicing vidioc_enum_fmt fd = " << cf->efd();
>
> -
Where does these spurious empty lines come from ?
> if (!validateBufferType(arg->type) ||
> arg->index >= streamConfig_.formats().pixelformats().size())
> return -EINVAL;
> @@ -255,7 +255,6 @@ int V4L2CameraProxy::vidioc_g_fmt(V4L2CameraFile *cf, struct v4l2_format *arg)
> {
> LOG(V4L2Compat, Debug) << "Servicing vidioc_g_fmt fd = " << cf->efd();
>
> -
> if (!validateBufferType(arg->type))
> return -EINVAL;
>
> @@ -543,8 +542,32 @@ int V4L2CameraProxy::vidioc_streamoff(V4L2CameraFile *cf, int *arg)
> return ret;
> }
>
> +std::set<unsigned long> V4L2CameraProxy::supportedIoctls_ = {
> + VIDIOC_QUERYCAP,
> + VIDIOC_ENUM_FMT,
> + VIDIOC_G_FMT,
> + VIDIOC_S_FMT,
> + VIDIOC_TRY_FMT,
> + VIDIOC_REQBUFS,
> + VIDIOC_QUERYBUF,
> + VIDIOC_QBUF,
> + VIDIOC_DQBUF,
> + VIDIOC_STREAMON,
> + VIDIOC_STREAMOFF,
> +};
I understand why an std::set(), as it provides a ::find() function
which ease lookup. I'm wondering if it's worth it though, as a
function in an anonymous namespace which simply switch on the
supported requests might be more efficient.
Anyway, have you considered having this in an anonymous namespace
instead of making a static class member out of it ? Is it accessed
from any other class (don't think so, as it's a private static class
member)
> +
> int V4L2CameraProxy::ioctl(V4L2CameraFile *cf, unsigned long request, void *arg)
> {
> + if (supportedIoctls_.find(request) == supportedIoctls_.end()) {
> + errno = ENOTTY;
> + return -1;
> + }
> +
> + if (!arg) {
> + errno = EFAULT;
> + return -1;
> + }
> +
> int ret;
> switch (request) {
> case VIDIOC_QUERYCAP:
> diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h
> index 43290ca..dd7e793 100644
> --- a/src/v4l2/v4l2_camera_proxy.h
> +++ b/src/v4l2/v4l2_camera_proxy.h
> @@ -11,6 +11,7 @@
> #include <linux/videodev2.h>
> #include <map>
> #include <memory>
> +#include <set>
> #include <sys/mman.h>
> #include <sys/types.h>
> #include <vector>
> @@ -67,6 +68,8 @@ private:
> static PixelFormat v4l2ToDrm(uint32_t format);
> static uint32_t drmToV4L2(const PixelFormat &format);
>
> + static std::set<unsigned long> supportedIoctls_;
> +
> unsigned int refcount_;
> unsigned int index_;
>
> --
> 2.27.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list