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

[Barnabás Pőcze]

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


> 
> > > +{
> > > +       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.


Regards,
Barnabás Pőcze


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