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

[Laurent Pinchart]

Hi Barnabás,

On Fri, Jun 21, 2024 at 01:55:43PM +0000, Barnabás Pőcze wrote:
> > 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.
> 
> If someone wants to rename the function or add a comment while committing, I am fine with that.
> Or should I send a new version?

I'll add the comment, no need to resend.

> > > > +{
> > > > +       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 :-)
> 
> Well, glibc has a check as well: https://sourceware.org/git/?p=glibc.git;a=blob;f=io/open_2.c;h=6466c36f5adeef9c59b494bd4de2fe30c740a4b4;hb=HEAD#l26
> So I don't think it hurts.

If glibc has those checks we may not need to duplicate them :-) But I'm
OK with the asserts.

> > > 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