[{"id":30031,"web_url":"https://patchwork.libcamera.org/comment/30031/","msgid":"<171896776945.2248009.7434318721670605459@ping.linuxembedded.co.uk>","date":"2024-06-21T11:02:49","subject":"Re: [PATCH v1] v4l2: v4l2_compat: Move `open*()` flag check into\n\tfunction","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Barnabás Pőcze (2024-06-18 02:06:55)\n> This commit moves the check that determines whether the\n> mode argument of `open*()` exists into a separate function.\n> \n> With that, the check is fixed because previously it failed to\n> account for the fact that `O_TMPFILE` is not a power of two.\n> \n> Furthermore, add `asserts()` in the fortified variants that\n> ensure that no mode is required by the specified flags.\n> \n> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>\n> ---\n>  src/v4l2/v4l2_compat.cpp | 18 ++++++++++++++----\n>  1 file changed, 14 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/v4l2/v4l2_compat.cpp b/src/v4l2/v4l2_compat.cpp\n> index 8a44403e..5c6f925e 100644\n> --- a/src/v4l2/v4l2_compat.cpp\n> +++ b/src/v4l2/v4l2_compat.cpp\n> @@ -7,6 +7,7 @@\n>  \n>  #include \"v4l2_compat_manager.h\"\n>  \n> +#include <assert.h>\n>  #include <errno.h>\n>  #include <fcntl.h>\n>  #include <stdarg.h>\n> @@ -28,12 +29,17 @@ using namespace libcamera;\n>         va_end(ap);                     \\\n>  }\n>  \n\nI'd be tempted to add a comment here explaining /what/ this check is\nfor. It wasn't obvious to me straight away.\n\n\n/*\n * Determine if the flags set will require a further mode flag to be\n * parsed as an additional argument in the va_args.\n */\n\n> +static inline bool needs_mode(int flags)\n> +{\n> +       return (flags & O_CREAT) || ((flags & O_TMPFILE) == O_TMPFILE);\n> +}\n> +\n>  extern \"C\" {\n>  \n>  LIBCAMERA_PUBLIC int open(const char *path, int oflag, ...)\n>  {\n>         mode_t mode = 0;\n> -       if (oflag & O_CREAT || oflag & O_TMPFILE)\n> +       if (needs_mode(oflag))\n>                 extract_va_arg(mode_t, mode, oflag);\n>  \n>         return V4L2CompatManager::instance()->openat(AT_FDCWD, path,\n> @@ -43,6 +49,7 @@ LIBCAMERA_PUBLIC int open(const char *path, int oflag, ...)\n>  /* _FORTIFY_SOURCE redirects open to __open_2 */\n>  LIBCAMERA_PUBLIC int __open_2(const char *path, int oflag)\n>  {\n> +       assert(!needs_mode(oflag));\n\nI was going to ask if we should be returning errors here instead, but\nnow I checked and I see we're in _FORTIFY_SOURCE where assertions are\nperfectly valid and expected even, so I think this is fine.\n\n\nReviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n\n>         return open(path, oflag);\n>  }\n>  \n> @@ -50,7 +57,7 @@ LIBCAMERA_PUBLIC int __open_2(const char *path, int oflag)\n>  LIBCAMERA_PUBLIC int open64(const char *path, int oflag, ...)\n>  {\n>         mode_t mode = 0;\n> -       if (oflag & O_CREAT || oflag & O_TMPFILE)\n> +       if (needs_mode(oflag))\n>                 extract_va_arg(mode_t, mode, oflag);\n>  \n>         return V4L2CompatManager::instance()->openat(AT_FDCWD, path,\n> @@ -59,6 +66,7 @@ LIBCAMERA_PUBLIC int open64(const char *path, int oflag, ...)\n>  \n>  LIBCAMERA_PUBLIC int __open64_2(const char *path, int oflag)\n>  {\n> +       assert(!needs_mode(oflag));\n>         return open64(path, oflag);\n>  }\n>  #endif\n> @@ -66,7 +74,7 @@ LIBCAMERA_PUBLIC int __open64_2(const char *path, int oflag)\n>  LIBCAMERA_PUBLIC int openat(int dirfd, const char *path, int oflag, ...)\n>  {\n>         mode_t mode = 0;\n> -       if (oflag & O_CREAT || oflag & O_TMPFILE)\n> +       if (needs_mode(oflag))\n>                 extract_va_arg(mode_t, mode, oflag);\n>  \n>         return V4L2CompatManager::instance()->openat(dirfd, path, oflag, mode);\n> @@ -74,6 +82,7 @@ LIBCAMERA_PUBLIC int openat(int dirfd, const char *path, int oflag, ...)\n>  \n>  LIBCAMERA_PUBLIC int __openat_2(int dirfd, const char *path, int oflag)\n>  {\n> +       assert(!needs_mode(oflag));\n>         return openat(dirfd, path, oflag);\n>  }\n>  \n> @@ -81,7 +90,7 @@ LIBCAMERA_PUBLIC int __openat_2(int dirfd, const char *path, int oflag)\n>  LIBCAMERA_PUBLIC int openat64(int dirfd, const char *path, int oflag, ...)\n>  {\n>         mode_t mode = 0;\n> -       if (oflag & O_CREAT || oflag & O_TMPFILE)\n> +       if (needs_mode(oflag))\n>                 extract_va_arg(mode_t, mode, oflag);\n>  \n>         return V4L2CompatManager::instance()->openat(dirfd, path,\n> @@ -90,6 +99,7 @@ LIBCAMERA_PUBLIC int openat64(int dirfd, const char *path, int oflag, ...)\n>  \n>  LIBCAMERA_PUBLIC int __openat64_2(int dirfd, const char *path, int oflag)\n>  {\n> +       assert(!needs_mode(oflag));\n>         return openat64(dirfd, path, oflag);\n>  }\n>  #endif\n> -- \n> 2.45.2\n> \n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5043CBE175\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 21 Jun 2024 11:02:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 70E42654A9;\n\tFri, 21 Jun 2024 13:02:54 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E6E79654A2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 21 Jun 2024 13:02:52 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 384C48E1;\n\tFri, 21 Jun 2024 13:02:33 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"bbGwVD43\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718967753;\n\tbh=oxxrRY/f2ZtLgZwH4ggq7Xl3u2qibpE35DboKgyT2tY=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=bbGwVD43z0CONZZVNEkSzfRHGVS9q8MWVVMd12YeErlVZxS64uJVQr9CuT/PYuLQb\n\tEcWLYWugVvh6cT7ofaC1fv3md3DDxoCiAUPFBCQY1tdIfgHR2P1U5fWd8NtTNqU2Q1\n\tR1g7NO0RBxWYm6I7keqHpP4FwnUveoTOQbWuCsOw=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240618010653.2346969-1-pobrn@protonmail.com>","References":"<20240618010653.2346969-1-pobrn@protonmail.com>","Subject":"Re: [PATCH v1] v4l2: v4l2_compat: Move `open*()` flag check into\n\tfunction","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <pobrn@protonmail.com>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 21 Jun 2024 12:02:49 +0100","Message-ID":"<171896776945.2248009.7434318721670605459@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":30035,"web_url":"https://patchwork.libcamera.org/comment/30035/","msgid":"<20240621134709.GL30640@pendragon.ideasonboard.com>","date":"2024-06-21T13:47:09","subject":"Re: [PATCH v1] v4l2: v4l2_compat: Move `open*()` flag check into\n\tfunction","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Jun 21, 2024 at 12:02:49PM +0100, Kieran Bingham wrote:\n> Quoting Barnabás Pőcze (2024-06-18 02:06:55)\n> > This commit moves the check that determines whether the\n> > mode argument of `open*()` exists into a separate function.\n> > \n> > With that, the check is fixed because previously it failed to\n> > account for the fact that `O_TMPFILE` is not a power of two.\n> > \n> > Furthermore, add `asserts()` in the fortified variants that\n> > ensure that no mode is required by the specified flags.\n> > \n> > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>\n> > ---\n> >  src/v4l2/v4l2_compat.cpp | 18 ++++++++++++++----\n> >  1 file changed, 14 insertions(+), 4 deletions(-)\n> > \n> > diff --git a/src/v4l2/v4l2_compat.cpp b/src/v4l2/v4l2_compat.cpp\n> > index 8a44403e..5c6f925e 100644\n> > --- a/src/v4l2/v4l2_compat.cpp\n> > +++ b/src/v4l2/v4l2_compat.cpp\n> > @@ -7,6 +7,7 @@\n> >  \n> >  #include \"v4l2_compat_manager.h\"\n> >  \n> > +#include <assert.h>\n> >  #include <errno.h>\n> >  #include <fcntl.h>\n> >  #include <stdarg.h>\n> > @@ -28,12 +29,17 @@ using namespace libcamera;\n> >         va_end(ap);                     \\\n> >  }\n> >  \n> \n> I'd be tempted to add a comment here explaining /what/ this check is\n> for. It wasn't obvious to me straight away.\n> \n> \n> /*\n>  * Determine if the flags set will require a further mode flag to be\n>  * parsed as an additional argument in the va_args.\n\nI had a bit of trouble parsing this. If you think the following is\nbetter you can use it.\n\n * Determine if the flags require a further mode arguments that needs to be\n * parsed from va_args.\n\n>  */\n> \n> > +static inline bool needs_mode(int flags)\n\nMaybe open_needs_mode() to match the name of the macro in fcntl.h ? Up\nto you.\n\n> > +{\n> > +       return (flags & O_CREAT) || ((flags & O_TMPFILE) == O_TMPFILE);\n> > +}\n> > +\n> >  extern \"C\" {\n> >  \n> >  LIBCAMERA_PUBLIC int open(const char *path, int oflag, ...)\n> >  {\n> >         mode_t mode = 0;\n> > -       if (oflag & O_CREAT || oflag & O_TMPFILE)\n> > +       if (needs_mode(oflag))\n> >                 extract_va_arg(mode_t, mode, oflag);\n> >  \n> >         return V4L2CompatManager::instance()->openat(AT_FDCWD, path,\n> > @@ -43,6 +49,7 @@ LIBCAMERA_PUBLIC int open(const char *path, int oflag, ...)\n> >  /* _FORTIFY_SOURCE redirects open to __open_2 */\n> >  LIBCAMERA_PUBLIC int __open_2(const char *path, int oflag)\n> >  {\n> > +       assert(!needs_mode(oflag));\n> \n> I was going to ask if we should be returning errors here instead, but\n> now I checked and I see we're in _FORTIFY_SOURCE where assertions are\n> perfectly valid and expected even, so I think this is fine.\n\nIs this check needed though ? FORTIFY_SOURCE already catches this issue,\nso I don't think we can be called with O_CREAT or O_TMPFILE here, the\ncode wouldn't have compiled in the first place. Of course someone could\ncall __open_2() manually without going through the FORTIFY_SOURCE open()\nwrapper, but in that case I think they don't deserve us caring :-)\n\n> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> \n> >         return open(path, oflag);\n> >  }\n> >  \n> > @@ -50,7 +57,7 @@ LIBCAMERA_PUBLIC int __open_2(const char *path, int oflag)\n> >  LIBCAMERA_PUBLIC int open64(const char *path, int oflag, ...)\n> >  {\n> >         mode_t mode = 0;\n> > -       if (oflag & O_CREAT || oflag & O_TMPFILE)\n> > +       if (needs_mode(oflag))\n> >                 extract_va_arg(mode_t, mode, oflag);\n> >  \n> >         return V4L2CompatManager::instance()->openat(AT_FDCWD, path,\n> > @@ -59,6 +66,7 @@ LIBCAMERA_PUBLIC int open64(const char *path, int oflag, ...)\n> >  \n> >  LIBCAMERA_PUBLIC int __open64_2(const char *path, int oflag)\n> >  {\n> > +       assert(!needs_mode(oflag));\n> >         return open64(path, oflag);\n> >  }\n> >  #endif\n> > @@ -66,7 +74,7 @@ LIBCAMERA_PUBLIC int __open64_2(const char *path, int oflag)\n> >  LIBCAMERA_PUBLIC int openat(int dirfd, const char *path, int oflag, ...)\n> >  {\n> >         mode_t mode = 0;\n> > -       if (oflag & O_CREAT || oflag & O_TMPFILE)\n> > +       if (needs_mode(oflag))\n> >                 extract_va_arg(mode_t, mode, oflag);\n> >  \n> >         return V4L2CompatManager::instance()->openat(dirfd, path, oflag, mode);\n> > @@ -74,6 +82,7 @@ LIBCAMERA_PUBLIC int openat(int dirfd, const char *path, int oflag, ...)\n> >  \n> >  LIBCAMERA_PUBLIC int __openat_2(int dirfd, const char *path, int oflag)\n> >  {\n> > +       assert(!needs_mode(oflag));\n> >         return openat(dirfd, path, oflag);\n> >  }\n> >  \n> > @@ -81,7 +90,7 @@ LIBCAMERA_PUBLIC int __openat_2(int dirfd, const char *path, int oflag)\n> >  LIBCAMERA_PUBLIC int openat64(int dirfd, const char *path, int oflag, ...)\n> >  {\n> >         mode_t mode = 0;\n> > -       if (oflag & O_CREAT || oflag & O_TMPFILE)\n> > +       if (needs_mode(oflag))\n> >                 extract_va_arg(mode_t, mode, oflag);\n> >  \n> >         return V4L2CompatManager::instance()->openat(dirfd, path,\n> > @@ -90,6 +99,7 @@ LIBCAMERA_PUBLIC int openat64(int dirfd, const char *path, int oflag, ...)\n> >  \n> >  LIBCAMERA_PUBLIC int __openat64_2(int dirfd, const char *path, int oflag)\n> >  {\n> > +       assert(!needs_mode(oflag));\n> >         return openat64(dirfd, path, oflag);\n> >  }\n> >  #endif\n> >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9E934BE175\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 21 Jun 2024 13:47:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 886B8654A9;\n\tFri, 21 Jun 2024 15:47:33 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C2F0E654A2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 21 Jun 2024 15:47:31 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BCC148E1;\n\tFri, 21 Jun 2024 15:47:11 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"sqOBiIf0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718977631;\n\tbh=kd3/1aBAOSQxCPv5YwhAoc4R3YK2JqTYcnSy4EaOGi0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sqOBiIf07+pnkBWEm6eKFDnl9/SCqxv5MsYRT7y4kbybPYL0trPc+fk84a3NSmIg1\n\tjZ9QRScTcp0lEjw/nG2kSBAxI6TO1qvKAK8pqIcwSlDCfh22Fs39EC/Ia1XwM6XSk2\n\tcLJ/+gMnLMFa8aSCwk0PkP1iCX/83a5eSLrAN/lQ=","Date":"Fri, 21 Jun 2024 16:47:09 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <pobrn@protonmail.com>,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1] v4l2: v4l2_compat: Move `open*()` flag check into\n\tfunction","Message-ID":"<20240621134709.GL30640@pendragon.ideasonboard.com>","References":"<20240618010653.2346969-1-pobrn@protonmail.com>\n\t<171896776945.2248009.7434318721670605459@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<171896776945.2248009.7434318721670605459@ping.linuxembedded.co.uk>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":30037,"web_url":"https://patchwork.libcamera.org/comment/30037/","msgid":"<2pofuKedGhqQ2LIQohma7E8B3EJtX07KvKkubcBf_gB6xIvj8Bsv32WIlQaBeyc9Ly0r2yzJ3cFc61As-YsZS0Qb04wPCMU1gVb121FRxIY=@protonmail.com>","date":"2024-06-21T13:55:43","subject":"Re: [PATCH v1] v4l2: v4l2_compat: Move `open*()` flag check into\n\tfunction","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"> On Fri, Jun 21, 2024 at 12:02:49PM +0100, Kieran Bingham wrote:\n> > Quoting Barnabás Pőcze (2024-06-18 02:06:55)\n> > > This commit moves the check that determines whether the\n> > > mode argument of `open*()` exists into a separate function.\n> > >\n> > > With that, the check is fixed because previously it failed to\n> > > account for the fact that `O_TMPFILE` is not a power of two.\n> > >\n> > > Furthermore, add `asserts()` in the fortified variants that\n> > > ensure that no mode is required by the specified flags.\n> > >\n> > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>\n> > > ---\n> > >  src/v4l2/v4l2_compat.cpp | 18 ++++++++++++++----\n> > >  1 file changed, 14 insertions(+), 4 deletions(-)\n> > >\n> > > diff --git a/src/v4l2/v4l2_compat.cpp b/src/v4l2/v4l2_compat.cpp\n> > > index 8a44403e..5c6f925e 100644\n> > > --- a/src/v4l2/v4l2_compat.cpp\n> > > +++ b/src/v4l2/v4l2_compat.cpp\n> > > @@ -7,6 +7,7 @@\n> > >\n> > >  #include \"v4l2_compat_manager.h\"\n> > >\n> > > +#include <assert.h>\n> > >  #include <errno.h>\n> > >  #include <fcntl.h>\n> > >  #include <stdarg.h>\n> > > @@ -28,12 +29,17 @@ using namespace libcamera;\n> > >         va_end(ap);                     \\\n> > >  }\n> > >\n> >\n> > I'd be tempted to add a comment here explaining /what/ this check is\n> > for. It wasn't obvious to me straight away.\n> >\n> >\n> > /*\n> >  * Determine if the flags set will require a further mode flag to be\n> >  * parsed as an additional argument in the va_args.\n> \n> I had a bit of trouble parsing this. If you think the following is\n> better you can use it.\n> \n>  * Determine if the flags require a further mode arguments that needs to be\n>  * parsed from va_args.\n> \n> >  */\n> >\n> > > +static inline bool needs_mode(int flags)\n> \n> Maybe open_needs_mode() to match the name of the macro in fcntl.h ? Up\n> to you.\n\nIf someone wants to rename the function or add a comment while committing, I am fine with that.\nOr should I send a new version?\n\n\n> \n> > > +{\n> > > +       return (flags & O_CREAT) || ((flags & O_TMPFILE) == O_TMPFILE);\n> > > +}\n> > > +\n> > >  extern \"C\" {\n> > >\n> > >  LIBCAMERA_PUBLIC int open(const char *path, int oflag, ...)\n> > >  {\n> > >         mode_t mode = 0;\n> > > -       if (oflag & O_CREAT || oflag & O_TMPFILE)\n> > > +       if (needs_mode(oflag))\n> > >                 extract_va_arg(mode_t, mode, oflag);\n> > >\n> > >         return V4L2CompatManager::instance()->openat(AT_FDCWD, path,\n> > > @@ -43,6 +49,7 @@ LIBCAMERA_PUBLIC int open(const char *path, int oflag, ...)\n> > >  /* _FORTIFY_SOURCE redirects open to __open_2 */\n> > >  LIBCAMERA_PUBLIC int __open_2(const char *path, int oflag)\n> > >  {\n> > > +       assert(!needs_mode(oflag));\n> >\n> > I was going to ask if we should be returning errors here instead, but\n> > now I checked and I see we're in _FORTIFY_SOURCE where assertions are\n> > perfectly valid and expected even, so I think this is fine.\n> \n> Is this check needed though ? FORTIFY_SOURCE already catches this issue,\n> so I don't think we can be called with O_CREAT or O_TMPFILE here, the\n> code wouldn't have compiled in the first place. Of course someone could\n> call __open_2() manually without going through the FORTIFY_SOURCE open()\n> wrapper, but in that case I think they don't deserve us caring :-)\n\nWell, 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\nSo I don't think it hurts.\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> >\n> > >         return open(path, oflag);\n> > >  }\n> > >\n> > > @@ -50,7 +57,7 @@ LIBCAMERA_PUBLIC int __open_2(const char *path, int oflag)\n> > >  LIBCAMERA_PUBLIC int open64(const char *path, int oflag, ...)\n> > >  {\n> > >         mode_t mode = 0;\n> > > -       if (oflag & O_CREAT || oflag & O_TMPFILE)\n> > > +       if (needs_mode(oflag))\n> > >                 extract_va_arg(mode_t, mode, oflag);\n> > >\n> > >         return V4L2CompatManager::instance()->openat(AT_FDCWD, path,\n> > > @@ -59,6 +66,7 @@ LIBCAMERA_PUBLIC int open64(const char *path, int oflag, ...)\n> > >\n> > >  LIBCAMERA_PUBLIC int __open64_2(const char *path, int oflag)\n> > >  {\n> > > +       assert(!needs_mode(oflag));\n> > >         return open64(path, oflag);\n> > >  }\n> > >  #endif\n> > > @@ -66,7 +74,7 @@ LIBCAMERA_PUBLIC int __open64_2(const char *path, int oflag)\n> > >  LIBCAMERA_PUBLIC int openat(int dirfd, const char *path, int oflag, ...)\n> > >  {\n> > >         mode_t mode = 0;\n> > > -       if (oflag & O_CREAT || oflag & O_TMPFILE)\n> > > +       if (needs_mode(oflag))\n> > >                 extract_va_arg(mode_t, mode, oflag);\n> > >\n> > >         return V4L2CompatManager::instance()->openat(dirfd, path, oflag, mode);\n> > > @@ -74,6 +82,7 @@ LIBCAMERA_PUBLIC int openat(int dirfd, const char *path, int oflag, ...)\n> > >\n> > >  LIBCAMERA_PUBLIC int __openat_2(int dirfd, const char *path, int oflag)\n> > >  {\n> > > +       assert(!needs_mode(oflag));\n> > >         return openat(dirfd, path, oflag);\n> > >  }\n> > >\n> > > @@ -81,7 +90,7 @@ LIBCAMERA_PUBLIC int __openat_2(int dirfd, const char *path, int oflag)\n> > >  LIBCAMERA_PUBLIC int openat64(int dirfd, const char *path, int oflag, ...)\n> > >  {\n> > >         mode_t mode = 0;\n> > > -       if (oflag & O_CREAT || oflag & O_TMPFILE)\n> > > +       if (needs_mode(oflag))\n> > >                 extract_va_arg(mode_t, mode, oflag);\n> > >\n> > >         return V4L2CompatManager::instance()->openat(dirfd, path,\n> > > @@ -90,6 +99,7 @@ LIBCAMERA_PUBLIC int openat64(int dirfd, const char *path, int oflag, ...)\n> > >\n> > >  LIBCAMERA_PUBLIC int __openat64_2(int dirfd, const char *path, int oflag)\n> > >  {\n> > > +       assert(!needs_mode(oflag));\n> > >         return openat64(dirfd, path, oflag);\n> > >  }\n> > >  #endif\n> > >\n> \n> --\n> Regards,\n> \n> Laurent Pinchart\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C7869BE175\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 21 Jun 2024 13:55:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DD6E6654AC;\n\tFri, 21 Jun 2024 15:55:49 +0200 (CEST)","from mail-4316.protonmail.ch (mail-4316.protonmail.ch\n\t[185.70.43.16])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3944A654A2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 21 Jun 2024 15:55:48 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"s5/9zpjA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1718978147; x=1719237347;\n\tbh=PNA1UZFLFDmAmrjrEButELofBSF4O9PAm1r4mi73OoQ=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector;\n\tb=s5/9zpjAHmlTsW2rYY0jCld9HIYfEBARhHzrvoR1jPU0E5VGNjPPMYJS5f10d8cgS\n\t1LEGfSVCK8DHsGVeasJ3vEOIDOLTRxaU/5hEp+qTOB4YEYIUIgbL4prHalrImKYKqE\n\tmxuHkCrB5/jEItCK4ZcgS5/yH4bLyb/jQiAISJapJ/AbIuLet0qt0Q0nE2g/ORtEdM\n\tPUGF9Z5szhb4uJa0cUEdVEseRaw/7PHftKhGXrcs691adku5nyAFMP7lJMFAoje5Yl\n\tLXropzDzf2m5lU97D/gfUQaN77VRt4+i1WFMNjniMgnEnZHlHx9E/AmhJ4dBSLdd4f\n\twFo+4HeVw2VEg==","Date":"Fri, 21 Jun 2024 13:55:43 +0000","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <pobrn@protonmail.com>","Cc":"Kieran Bingham <kieran.bingham@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1] v4l2: v4l2_compat: Move `open*()` flag check into\n\tfunction","Message-ID":"<2pofuKedGhqQ2LIQohma7E8B3EJtX07KvKkubcBf_gB6xIvj8Bsv32WIlQaBeyc9Ly0r2yzJ3cFc61As-YsZS0Qb04wPCMU1gVb121FRxIY=@protonmail.com>","In-Reply-To":"<20240621134709.GL30640@pendragon.ideasonboard.com>","References":"<20240618010653.2346969-1-pobrn@protonmail.com>\n\t<171896776945.2248009.7434318721670605459@ping.linuxembedded.co.uk>\n\t<20240621134709.GL30640@pendragon.ideasonboard.com>","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"8747c58bcaf181d92396c218bd69c7937d3c2fc2","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":30068,"web_url":"https://patchwork.libcamera.org/comment/30068/","msgid":"<20240625070458.GA31369@pendragon.ideasonboard.com>","date":"2024-06-25T07:04:58","subject":"Re: [PATCH v1] v4l2: v4l2_compat: Move `open*()` flag check into\n\tfunction","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Barnabás,\n\nOn Fri, Jun 21, 2024 at 01:55:43PM +0000, Barnabás Pőcze wrote:\n> > On Fri, Jun 21, 2024 at 12:02:49PM +0100, Kieran Bingham wrote:\n> > > Quoting Barnabás Pőcze (2024-06-18 02:06:55)\n> > > > This commit moves the check that determines whether the\n> > > > mode argument of `open*()` exists into a separate function.\n> > > >\n> > > > With that, the check is fixed because previously it failed to\n> > > > account for the fact that `O_TMPFILE` is not a power of two.\n> > > >\n> > > > Furthermore, add `asserts()` in the fortified variants that\n> > > > ensure that no mode is required by the specified flags.\n> > > >\n> > > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>\n> > > > ---\n> > > >  src/v4l2/v4l2_compat.cpp | 18 ++++++++++++++----\n> > > >  1 file changed, 14 insertions(+), 4 deletions(-)\n> > > >\n> > > > diff --git a/src/v4l2/v4l2_compat.cpp b/src/v4l2/v4l2_compat.cpp\n> > > > index 8a44403e..5c6f925e 100644\n> > > > --- a/src/v4l2/v4l2_compat.cpp\n> > > > +++ b/src/v4l2/v4l2_compat.cpp\n> > > > @@ -7,6 +7,7 @@\n> > > >\n> > > >  #include \"v4l2_compat_manager.h\"\n> > > >\n> > > > +#include <assert.h>\n> > > >  #include <errno.h>\n> > > >  #include <fcntl.h>\n> > > >  #include <stdarg.h>\n> > > > @@ -28,12 +29,17 @@ using namespace libcamera;\n> > > >         va_end(ap);                     \\\n> > > >  }\n> > > >\n> > >\n> > > I'd be tempted to add a comment here explaining /what/ this check is\n> > > for. It wasn't obvious to me straight away.\n> > >\n> > > /*\n> > >  * Determine if the flags set will require a further mode flag to be\n> > >  * parsed as an additional argument in the va_args.\n> > \n> > I had a bit of trouble parsing this. If you think the following is\n> > better you can use it.\n> > \n> >  * Determine if the flags require a further mode arguments that needs to be\n> >  * parsed from va_args.\n> > \n> > >  */\n> > >\n> > > > +static inline bool needs_mode(int flags)\n> > \n> > Maybe open_needs_mode() to match the name of the macro in fcntl.h ? Up\n> > to you.\n> \n> If someone wants to rename the function or add a comment while committing, I am fine with that.\n> Or should I send a new version?\n\nI'll add the comment, no need to resend.\n\n> > > > +{\n> > > > +       return (flags & O_CREAT) || ((flags & O_TMPFILE) == O_TMPFILE);\n> > > > +}\n> > > > +\n> > > >  extern \"C\" {\n> > > >\n> > > >  LIBCAMERA_PUBLIC int open(const char *path, int oflag, ...)\n> > > >  {\n> > > >         mode_t mode = 0;\n> > > > -       if (oflag & O_CREAT || oflag & O_TMPFILE)\n> > > > +       if (needs_mode(oflag))\n> > > >                 extract_va_arg(mode_t, mode, oflag);\n> > > >\n> > > >         return V4L2CompatManager::instance()->openat(AT_FDCWD, path,\n> > > > @@ -43,6 +49,7 @@ LIBCAMERA_PUBLIC int open(const char *path, int oflag, ...)\n> > > >  /* _FORTIFY_SOURCE redirects open to __open_2 */\n> > > >  LIBCAMERA_PUBLIC int __open_2(const char *path, int oflag)\n> > > >  {\n> > > > +       assert(!needs_mode(oflag));\n> > >\n> > > I was going to ask if we should be returning errors here instead, but\n> > > now I checked and I see we're in _FORTIFY_SOURCE where assertions are\n> > > perfectly valid and expected even, so I think this is fine.\n> > \n> > Is this check needed though ? FORTIFY_SOURCE already catches this issue,\n> > so I don't think we can be called with O_CREAT or O_TMPFILE here, the\n> > code wouldn't have compiled in the first place. Of course someone could\n> > call __open_2() manually without going through the FORTIFY_SOURCE open()\n> > wrapper, but in that case I think they don't deserve us caring :-)\n> \n> 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\n> So I don't think it hurts.\n\nIf glibc has those checks we may not need to duplicate them :-) But I'm\nOK with the asserts.\n\n> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> > >\n> > > >         return open(path, oflag);\n> > > >  }\n> > > >\n> > > > @@ -50,7 +57,7 @@ LIBCAMERA_PUBLIC int __open_2(const char *path, int oflag)\n> > > >  LIBCAMERA_PUBLIC int open64(const char *path, int oflag, ...)\n> > > >  {\n> > > >         mode_t mode = 0;\n> > > > -       if (oflag & O_CREAT || oflag & O_TMPFILE)\n> > > > +       if (needs_mode(oflag))\n> > > >                 extract_va_arg(mode_t, mode, oflag);\n> > > >\n> > > >         return V4L2CompatManager::instance()->openat(AT_FDCWD, path,\n> > > > @@ -59,6 +66,7 @@ LIBCAMERA_PUBLIC int open64(const char *path, int oflag, ...)\n> > > >\n> > > >  LIBCAMERA_PUBLIC int __open64_2(const char *path, int oflag)\n> > > >  {\n> > > > +       assert(!needs_mode(oflag));\n> > > >         return open64(path, oflag);\n> > > >  }\n> > > >  #endif\n> > > > @@ -66,7 +74,7 @@ LIBCAMERA_PUBLIC int __open64_2(const char *path, int oflag)\n> > > >  LIBCAMERA_PUBLIC int openat(int dirfd, const char *path, int oflag, ...)\n> > > >  {\n> > > >         mode_t mode = 0;\n> > > > -       if (oflag & O_CREAT || oflag & O_TMPFILE)\n> > > > +       if (needs_mode(oflag))\n> > > >                 extract_va_arg(mode_t, mode, oflag);\n> > > >\n> > > >         return V4L2CompatManager::instance()->openat(dirfd, path, oflag, mode);\n> > > > @@ -74,6 +82,7 @@ LIBCAMERA_PUBLIC int openat(int dirfd, const char *path, int oflag, ...)\n> > > >\n> > > >  LIBCAMERA_PUBLIC int __openat_2(int dirfd, const char *path, int oflag)\n> > > >  {\n> > > > +       assert(!needs_mode(oflag));\n> > > >         return openat(dirfd, path, oflag);\n> > > >  }\n> > > >\n> > > > @@ -81,7 +90,7 @@ LIBCAMERA_PUBLIC int __openat_2(int dirfd, const char *path, int oflag)\n> > > >  LIBCAMERA_PUBLIC int openat64(int dirfd, const char *path, int oflag, ...)\n> > > >  {\n> > > >         mode_t mode = 0;\n> > > > -       if (oflag & O_CREAT || oflag & O_TMPFILE)\n> > > > +       if (needs_mode(oflag))\n> > > >                 extract_va_arg(mode_t, mode, oflag);\n> > > >\n> > > >         return V4L2CompatManager::instance()->openat(dirfd, path,\n> > > > @@ -90,6 +99,7 @@ LIBCAMERA_PUBLIC int openat64(int dirfd, const char *path, int oflag, ...)\n> > > >\n> > > >  LIBCAMERA_PUBLIC int __openat64_2(int dirfd, const char *path, int oflag)\n> > > >  {\n> > > > +       assert(!needs_mode(oflag));\n> > > >         return openat64(dirfd, path, oflag);\n> > > >  }\n> > > >  #endif","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3879EBD87C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 25 Jun 2024 07:05:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5AACE654A1;\n\tTue, 25 Jun 2024 09:05:21 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A5C58619E8\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 25 Jun 2024 09:05:19 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [193.209.96.36])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0143CC62;\n\tTue, 25 Jun 2024 09:04:56 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"i/ssVEYr\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1719299097;\n\tbh=Vb1H7VOHPw9swdPgdg1PhGlcbgS/usHqbJwfG/Bbrlw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=i/ssVEYrWoVZaFL87ATAjw+6Vw+gTGG43I63v3DtctZzqUqF5VBDQ1uIsL8wfrMTP\n\tTGQXk6Jm12huhNFzkxyayHXzYtVCNZ9263rcyiPglZr413svGBe2wdVmtVkvHeE+k8\n\tc+Qiqs2/FOEI737R+yDzDiM1hLz04bZfVuSpZ9i4=","Date":"Tue, 25 Jun 2024 10:04:58 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <pobrn@protonmail.com>","Cc":"Kieran Bingham <kieran.bingham@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1] v4l2: v4l2_compat: Move `open*()` flag check into\n\tfunction","Message-ID":"<20240625070458.GA31369@pendragon.ideasonboard.com>","References":"<20240618010653.2346969-1-pobrn@protonmail.com>\n\t<171896776945.2248009.7434318721670605459@ping.linuxembedded.co.uk>\n\t<20240621134709.GL30640@pendragon.ideasonboard.com>\n\t<2pofuKedGhqQ2LIQohma7E8B3EJtX07KvKkubcBf_gB6xIvj8Bsv32WIlQaBeyc9Ly0r2yzJ3cFc61As-YsZS0Qb04wPCMU1gVb121FRxIY=@protonmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<2pofuKedGhqQ2LIQohma7E8B3EJtX07KvKkubcBf_gB6xIvj8Bsv32WIlQaBeyc9Ly0r2yzJ3cFc61As-YsZS0Qb04wPCMU1gVb121FRxIY=@protonmail.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]