[PATCH v1] v4l2: v4l2_compat: Move `open*()` flag check into function

[Laurent Pinchart]

On Fri, Jun 21, 2024 at 12:02:49PM +0100, Kieran Bingham wrote:
> Quoting Barnabás Pőcze (2024-06-18 02:06:55)
> > This commit moves the check that determines whether the
> > mode argument of `open*()` exists into a separate function.
> > 
> > With that, the check is fixed because previously it failed to
> > account for the fact that `O_TMPFILE` is not a power of two.
> > 
> > Furthermore, add `asserts()` in the fortified variants that
> > ensure that no mode is required by the specified flags.
> > 
> > Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>
> > ---
> >  src/v4l2/v4l2_compat.cpp | 18 ++++++++++++++----
> >  1 file changed, 14 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/v4l2/v4l2_compat.cpp b/src/v4l2/v4l2_compat.cpp
> > index 8a44403e..5c6f925e 100644
> > --- a/src/v4l2/v4l2_compat.cpp
> > +++ b/src/v4l2/v4l2_compat.cpp
> > @@ -7,6 +7,7 @@
> >  
> >  #include "v4l2_compat_manager.h"
> >  
> > +#include <assert.h>
> >  #include <errno.h>
> >  #include <fcntl.h>
> >  #include <stdarg.h>
> > @@ -28,12 +29,17 @@ using namespace libcamera;
> >         va_end(ap);                     \
> >  }
> >  
> 
> I'd be tempted to add a comment here explaining /what/ this check is
> for. It wasn't obvious to me straight away.
> 
> 
> /*
>  * Determine if the flags set will require a further mode flag to be
>  * parsed as an additional argument in the va_args.

I had a bit of trouble parsing this. If you think the following is
better you can use it.

 * Determine if the flags require a further mode arguments that needs to be
 * parsed from va_args.

>  */
> 
> > +static inline bool needs_mode(int flags)

Maybe open_needs_mode() to match the name of the macro in fcntl.h ? Up
to you.

> > +{
> > +       return (flags & O_CREAT) || ((flags & O_TMPFILE) == O_TMPFILE);
> > +}
> > +
> >  extern "C" {
> >  
> >  LIBCAMERA_PUBLIC int open(const char *path, int oflag, ...)
> >  {
> >         mode_t mode = 0;
> > -       if (oflag & O_CREAT || oflag & O_TMPFILE)
> > +       if (needs_mode(oflag))
> >                 extract_va_arg(mode_t, mode, oflag);
> >  
> >         return V4L2CompatManager::instance()->openat(AT_FDCWD, path,
> > @@ -43,6 +49,7 @@ LIBCAMERA_PUBLIC int open(const char *path, int oflag, ...)
> >  /* _FORTIFY_SOURCE redirects open to __open_2 */
> >  LIBCAMERA_PUBLIC int __open_2(const char *path, int oflag)
> >  {
> > +       assert(!needs_mode(oflag));
> 
> I was going to ask if we should be returning errors here instead, but
> now I checked and I see we're in _FORTIFY_SOURCE where assertions are
> perfectly valid and expected even, so I think this is fine.

Is this check needed though ? FORTIFY_SOURCE already catches this issue,
so I don't think we can be called with O_CREAT or O_TMPFILE here, the
code wouldn't have compiled in the first place. Of course someone could
call __open_2() manually without going through the FORTIFY_SOURCE open()
wrapper, but in that case I think they don't deserve us caring :-)

> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> >         return open(path, oflag);
> >  }
> >  
> > @@ -50,7 +57,7 @@ LIBCAMERA_PUBLIC int __open_2(const char *path, int oflag)
> >  LIBCAMERA_PUBLIC int open64(const char *path, int oflag, ...)
> >  {
> >         mode_t mode = 0;
> > -       if (oflag & O_CREAT || oflag & O_TMPFILE)
> > +       if (needs_mode(oflag))
> >                 extract_va_arg(mode_t, mode, oflag);
> >  
> >         return V4L2CompatManager::instance()->openat(AT_FDCWD, path,
> > @@ -59,6 +66,7 @@ LIBCAMERA_PUBLIC int open64(const char *path, int oflag, ...)
> >  
> >  LIBCAMERA_PUBLIC int __open64_2(const char *path, int oflag)
> >  {
> > +       assert(!needs_mode(oflag));
> >         return open64(path, oflag);
> >  }
> >  #endif
> > @@ -66,7 +74,7 @@ LIBCAMERA_PUBLIC int __open64_2(const char *path, int oflag)
> >  LIBCAMERA_PUBLIC int openat(int dirfd, const char *path, int oflag, ...)
> >  {
> >         mode_t mode = 0;
> > -       if (oflag & O_CREAT || oflag & O_TMPFILE)
> > +       if (needs_mode(oflag))
> >                 extract_va_arg(mode_t, mode, oflag);
> >  
> >         return V4L2CompatManager::instance()->openat(dirfd, path, oflag, mode);
> > @@ -74,6 +82,7 @@ LIBCAMERA_PUBLIC int openat(int dirfd, const char *path, int oflag, ...)
> >  
> >  LIBCAMERA_PUBLIC int __openat_2(int dirfd, const char *path, int oflag)
> >  {
> > +       assert(!needs_mode(oflag));
> >         return openat(dirfd, path, oflag);
> >  }
> >  
> > @@ -81,7 +90,7 @@ LIBCAMERA_PUBLIC int __openat_2(int dirfd, const char *path, int oflag)
> >  LIBCAMERA_PUBLIC int openat64(int dirfd, const char *path, int oflag, ...)
> >  {
> >         mode_t mode = 0;
> > -       if (oflag & O_CREAT || oflag & O_TMPFILE)
> > +       if (needs_mode(oflag))
> >                 extract_va_arg(mode_t, mode, oflag);
> >  
> >         return V4L2CompatManager::instance()->openat(dirfd, path,
> > @@ -90,6 +99,7 @@ LIBCAMERA_PUBLIC int openat64(int dirfd, const char *path, int oflag, ...)
> >  
> >  LIBCAMERA_PUBLIC int __openat64_2(int dirfd, const char *path, int oflag)
> >  {
> > +       assert(!needs_mode(oflag));
> >         return openat64(dirfd, path, oflag);
> >  }
> >  #endif
> >

-- 
Regards,

Laurent Pinchart