[PATCH v1] v4l2: v4l2_compat: Fix redirect from `__open(at)64_2()`

[Laurent Pinchart]

Hi Barnabás,

On Mon, Jun 17, 2024 at 11:54:53PM +0000, Barnabás Pőcze wrote:
> 2024. június 18., kedd 0:37 keltezéssel, Kieran Bingham írta:
> 
> > Quoting Barnabás Pőcze (2024-06-17 22:43:45)
> > > `__open64_2()` and `__openat64_2()` should redirect
> > > to `open64()` and `openat64()`, respectively, otherwise
> > > the `O_LARGEFILE` flag will not be applied.
> > >
> > > Fixes: 1023107b6405 ("v4l2: v4l2_compat: Intercept open64, openat64, and mmap64")
> > > Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>
> > 
> > CI is all green here.
> > https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1203630
> > 
> > But commit 1023107b6405 ("v4l2: v4l2_compat: Intercept open64, openat64, and mmap64")
> > states:
> > 
> >     Also, since we set _FILE_OFFSET_BITS to 32 to force the various open and
> >     mmap symbols that we export to not be the 64-bit versions, our dlsym to
> >     get the original open and mmap calls will not automatically be converted
> >     to their 64-bit versions. Since we intercept both 32-bit and 64-bit
> >     versions of open and mmap, we should be using the 64-bit version to
> >     service both. Fetch the 64-bit versions of openat and mmap directly.
> > 
> > is that why these were the non-64 bit variants?
> > 
> > This feels like it's "too obvious" ... I wonder what we missed ...
> > 
> > Paul? Any insights? (It was ... a long time ago that you wrote that
> > patch)
> 
> Looks like it is me who is confused. V4L2CompatManager::instance()->openat()
> indeed uses openat64 to service the call.

That's correct.

> And it would appear that both
> `openat64()` and `open64()` unconditionally add `O_LARGEFILE`, which, in hindsight,
> is the expected behaviour: https://codebrowser.dev/glibc/glibc/sysdeps/unix/sysv/linux/openat64.c.html#39

Aahhh I was missing that part.

> In this case, patch does not change a thing, apart from eliminating the
> visual inconsistency that caught my eyes in the first place.

It also adds O_LARGEFILE manually inside the V4L2 compat layer, which
avoids confusion, even if it doesn't change the behaviour.

Note that, while uclibc-ng seems to mimic the glibc implementation, musl
doesn't add O_LARGEFILE internally, so specifying it explicitly in the
compat layer is needed there.

> However, this raises a different issue: a call to open()/openat() will automatically get `O_LARGEFILE`:
> 
> $ cat asd.c
> #include <assert.h>
> #include <fcntl.h>
> #include <unistd.h>
> 
> int main(int argc, char *argv[])
> {
> 	assert(argc == 2);
> 	return close(open(argv[1], O_RDONLY));
> }
> $ gcc -m32 asd.c
> $ truncate -s 10G testfile
> 
> $ strace -e openat ./a.out testfile
> [...]
> openat(AT_FDCWD, "testfile", O_RDONLY)  = -1 EOVERFLOW (Value too large for defined data type)
> +++ exited with 255 +++
> 
> $ strace -e openat -E LD_PRELOAD=.../src/v4l2/v4l2-compat.so ./a.out testfile
> [...]
> openat(AT_FDCWD, "testfile", O_RDONLY|O_LARGEFILE) = 3
> +++ exited with 0 +++
> 
> So a file that shouldn't be opened can now be opened.

Indeed. To solve that I think we would need to dlsym() the openat
symbol, and call it for the 32-bit calls. I wonder if this issue is
worth fixing though.

I still think your patch is worth merging, but with an updated commit
message.

> > > ---
> > >  src/v4l2/v4l2_compat.cpp | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/v4l2/v4l2_compat.cpp b/src/v4l2/v4l2_compat.cpp
> > > index 8e2b7e92..8a44403e 100644
> > > --- a/src/v4l2/v4l2_compat.cpp
> > > +++ b/src/v4l2/v4l2_compat.cpp
> > > @@ -59,7 +59,7 @@ LIBCAMERA_PUBLIC int open64(const char *path, int oflag, ...)
> > >
> > >  LIBCAMERA_PUBLIC int __open64_2(const char *path, int oflag)
> > >  {
> > > -       return open(path, oflag);
> > > +       return open64(path, oflag);
> > >  }
> > >  #endif
> > >
> > > @@ -90,7 +90,7 @@ LIBCAMERA_PUBLIC int openat64(int dirfd, const char *path, int oflag, ...)
> > >
> > >  LIBCAMERA_PUBLIC int __openat64_2(int dirfd, const char *path, int oflag)
> > >  {
> > > -       return openat(dirfd, path, oflag);
> > > +       return openat64(dirfd, path, oflag);
> > >  }
> > >  #endif
> > >

-- 
Regards,

Laurent Pinchart