[PATCH] io: Fix record locking contants on 32 bit arch with 64 bit default time_t (BZ#30477)

[PATCH] io: Fix record locking contants on 32 bit arch with 64 bit default time_t (BZ#30477)

[Adhemerval Zanella]

For architecture with default 64 bit time_t support, the kernel
does not provide LFS and non-LFS values for F_GETLK, F_GETLK, and
F_GETLK (the default value used for 64 bit architecture are used).

This is might be considered an ABI break, but the currenct exported
values is bogus anyway.

The POSIX lockf is not affected since it is aliased to lockf64,
which already uses the LFS values.

Checked on i686-linux-gnu and the new tests on a riscv32.
---
 io/Makefile                                |   1 +
 io/tst-fcntl-lock.c                        | 218 +++++++++++++++++++++
 sysdeps/unix/sysv/linux/bits/fcntl-linux.h |   2 +-
 3 files changed, 220 insertions(+), 1 deletion(-)
 create mode 100644 io/tst-fcntl-lock.c

diff --git a/io/Makefile b/io/Makefile
index db886ca26f..d573064ecc 100644
--- a/io/Makefile
+++ b/io/Makefile
@@ -175,6 +175,7 @@ tests := \
   tst-fchmodat \
   tst-fchownat \
   tst-fcntl \
+  tst-fcntl-lock \
   tst-fstatat \
   tst-fts \
   tst-fts-lfs \
diff --git a/io/tst-fcntl-lock.c b/io/tst-fcntl-lock.c
new file mode 100644
index 0000000000..8735b147f7
--- /dev/null
+++ b/io/tst-fcntl-lock.c
@@ -0,0 +1,218 @@
+/* Test for advisory record locking.
+   Copyright (C) 2023 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or
+   modify it under the terms of the GNU General Public License
+   as published by the Free Software Foundation; either version 2
+   of the License, or (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program; if not, see <https://www.gnu.org/licenses/>.
+*/
+
+#include <fcntl.h>
+#include <unistd.h>
+#include <errno.h>
+#include <stdint.h>
+
+#include <support/temp_file.h>
+#include <support/capture_subprocess.h>
+#include <support/check.h>
+
+static char *temp_filename;
+static int temp_fd;
+
+static void
+do_prepare (int argc, char **argv)
+{
+  temp_fd = create_temp_file ("tst-fcntl-lock.", &temp_filename);
+  TEST_VERIFY_EXIT (temp_fd != -1);
+}
+#define PREPARE do_prepare
+
+/* This is essentially the POSIX lockf.  */
+
+enum fcntl_lock_cmd
+{
+  TEST,
+  ULOCK,
+  LOCK,
+  TLOCK,
+};
+
+static inline __attribute_artificial__ int
+fcntl_lockf (int fd, enum fcntl_lock_cmd cmd, off_t len)
+{
+  struct flock fl = {
+    .l_type = F_WRLCK,
+    .l_whence = SEEK_CUR,
+    .l_len = len
+  };
+
+  switch (cmd)
+    {
+    case TEST:
+      fl.l_type = F_RDLCK;
+      if (fcntl (fd, F_GETLK, &fl) < 0)
+	return -1;
+      if (fl.l_type == F_UNLCK || fl.l_pid == getpid ())
+	return 0;
+      errno = EACCES;
+      return -1;
+
+    case ULOCK:
+      fl.l_type = F_UNLCK;
+      return fcntl (fd, F_SETLK, &fl);
+
+    case LOCK:
+      return fcntl (fd, F_SETLKW, &fl);
+
+    case TLOCK:
+      return fcntl (fd, F_SETLK, &fl);
+    }
+
+  errno = EINVAL;
+  return -1;
+}
+
+static inline __attribute_artificial__ int
+fcntl64_lockf (int fd, enum fcntl_lock_cmd cmd, off64_t len64)
+{
+  struct flock64 fl64 = {
+    .l_type = F_WRLCK,
+    .l_whence = SEEK_CUR,
+    .l_len = len64
+  };
+
+  switch (cmd)
+    {
+    case TEST:
+      fl64.l_type = F_RDLCK;
+      if (fcntl64 (fd, F_GETLK64, &fl64) < 0)
+	return -1;
+      if (fl64.l_type == F_UNLCK || fl64.l_pid == getpid ())
+	return 0;
+      errno = EACCES;
+      return -1;
+
+    case ULOCK:
+      fl64.l_type = F_UNLCK;
+      return fcntl64 (fd, F_SETLK64, &fl64);
+
+    case LOCK:
+      return fcntl64 (fd, F_SETLKW64, &fl64);
+
+    case TLOCK:
+      return fcntl64 (fd, F_SETLK64, &fl64);
+    }
+
+  errno = EINVAL;
+  return -1;
+}
+
+static void
+do_test_child_lockf (void *closure)
+{
+  /* Check if parent has [0, 1024) locked.  */
+  TEST_COMPARE (lseek (temp_fd, 0, SEEK_SET), 0);
+  TEST_COMPARE (fcntl_lockf (temp_fd, TLOCK, 1024), -1);
+  TEST_COMPARE (errno, EAGAIN);
+  TEST_COMPARE (fcntl_lockf (temp_fd, TEST, 1024), -1);
+  TEST_COMPARE (errno, EACCES);
+  /* Also Check if parent has last 1024 bytes locked.  */
+  TEST_COMPARE (lseek (temp_fd, INT32_MAX-1024, SEEK_SET), INT32_MAX-1024);
+  TEST_COMPARE (fcntl_lockf (temp_fd, TEST, 1024), -1);
+
+  /* And try to lock [1024, 2048).  */
+  TEST_COMPARE (lseek (temp_fd, 1024, SEEK_SET), 1024);
+  TEST_COMPARE (fcntl_lockf (temp_fd, LOCK, 1024), 0);
+
+  /* Check if non-LFS interface cap access to 32-bif off_t.  */
+  TEST_COMPARE (lseek64 (temp_fd, (off64_t)INT32_MAX, SEEK_SET),
+		(off64_t)INT32_MAX);
+  TEST_COMPARE (fcntl64_lockf (temp_fd, TEST, 1024), 0);
+}
+
+static void
+do_test_child_lockf64 (void *closure)
+{
+  /* Check if parent has [0, 1024) locked.  */
+  TEST_COMPARE (lseek64 (temp_fd, 0, SEEK_SET), 0);
+  TEST_COMPARE (fcntl64_lockf (temp_fd, TLOCK, 1024), -1);
+  TEST_COMPARE (errno, EAGAIN);
+  TEST_COMPARE (fcntl64_lockf (temp_fd, TEST, 1024), -1);
+  TEST_COMPARE (errno, EACCES);
+  /* Also Check if parent has last 1024 bytes locked.  */
+  TEST_COMPARE (lseek64 (temp_fd, INT32_MAX-1024, SEEK_SET), INT32_MAX-1024);
+  TEST_COMPARE (fcntl64_lockf (temp_fd, TEST, 1024), -1);
+
+  /* And try to lock [1024, 2048).  */
+  TEST_COMPARE (lseek64 (temp_fd, 1024, SEEK_SET), 1024);
+  TEST_COMPARE (fcntl64_lockf (temp_fd, LOCK, 1024), 0);
+
+  /* And also [INT32_MAX, INT32_MAX+1024).  */
+  {
+    off64_t off = (off64_t)INT32_MAX;
+    TEST_COMPARE (lseek64 (temp_fd, off, SEEK_SET), off);
+    TEST_COMPARE (fcntl64_lockf (temp_fd, LOCK, 1024), 0);
+  }
+
+  /* Check if [INT32_MAX+1024, INT64_MAX) is locked.  */
+  {
+    off64_t off = (off64_t)INT32_MAX+1024;
+    TEST_COMPARE (lseek64 (temp_fd, off, SEEK_SET), off);
+    TEST_COMPARE (fcntl64_lockf (temp_fd, TLOCK, 1024), -1);
+    TEST_COMPARE (errno, EAGAIN);
+    TEST_COMPARE (fcntl64_lockf (temp_fd, TEST, 1024), -1);
+    TEST_COMPARE (errno, EACCES);
+  }
+}
+
+static int
+do_test (void)
+{
+  /* Basic tests to check if a lock can be obtained and checked.  */
+  TEST_COMPARE (fcntl_lockf (temp_fd, LOCK, 1024), 0);
+  TEST_COMPARE (fcntl_lockf (temp_fd, LOCK, INT32_MAX), 0);
+  TEST_COMPARE (fcntl_lockf (temp_fd, TLOCK, 1024), 0);
+  TEST_COMPARE (fcntl_lockf (temp_fd, TEST, 1024), 0);
+  TEST_COMPARE (lseek (temp_fd, 1024, SEEK_SET), 1024);
+  TEST_COMPARE (fcntl_lockf (temp_fd, ULOCK, 1024), 0);
+  /* Parent process should have ([0, 1024), [2048, INT32_MAX)) ranges locked.  */
+
+  {
+    struct support_capture_subprocess result;
+    result = support_capture_subprocess (do_test_child_lockf, NULL);
+    support_capture_subprocess_check (&result, "lockf", 0, sc_allow_none);
+  }
+
+  if (sizeof (off_t) != sizeof (off64_t))
+    {
+      /* Check if previously locked regions with LFS symbol.  */
+      TEST_COMPARE (lseek (temp_fd, 0, SEEK_SET), 0);
+      TEST_COMPARE (fcntl64_lockf (temp_fd, LOCK, 1024), 0);
+      TEST_COMPARE (fcntl64_lockf (temp_fd, TLOCK, 1024), 0);
+      TEST_COMPARE (fcntl64_lockf (temp_fd, TEST, 1024), 0);
+      /* Lock region [INT32_MAX+1024, INT64_MAX).  */
+      off64_t off = (off64_t)INT32_MAX + 1024;
+      TEST_COMPARE (lseek64 (temp_fd, off, SEEK_SET), off);
+      TEST_COMPARE (fcntl64_lockf (temp_fd, LOCK, 1024), 0);
+      /* Parent process should have ([0, 1024), [2048, INT32_MAX),
+	 [INT32_MAX+1024, INT64_MAX)) ranges locked.  */
+
+      {
+	struct support_capture_subprocess result;
+	result = support_capture_subprocess (do_test_child_lockf64, NULL);
+	support_capture_subprocess_check (&result, "lockf", 0, sc_allow_none);
+      }
+    }
+
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/sysdeps/unix/sysv/linux/bits/fcntl-linux.h b/sysdeps/unix/sysv/linux/bits/fcntl-linux.h
index ca6a0d7516..3c15625599 100644
--- a/sysdeps/unix/sysv/linux/bits/fcntl-linux.h
+++ b/sysdeps/unix/sysv/linux/bits/fcntl-linux.h
@@ -101,7 +101,7 @@
 #endif
 
 #ifndef F_GETLK
-# ifndef __USE_FILE_OFFSET64
+# if !defined(__USE_FILE_OFFSET64) && __TIMESIZE != 64
 #  define F_GETLK	5	/* Get record locking info.  */
 #  define F_SETLK	6	/* Set record locking info (non-blocking).  */
 #  define F_SETLKW	7	/* Set record locking info (blocking).  */
-- 
2.34.1

Re: [PATCH] io: Fix record locking contants on 32 bit arch with 64 bit default time_t (BZ#30477)

[Florian Weimer]

* Adhemerval Zanella via Libc-alpha:

> diff --git a/sysdeps/unix/sysv/linux/bits/fcntl-linux.h b/sysdeps/unix/sysv/linux/bits/fcntl-linux.h
> index ca6a0d7516..3c15625599 100644
> --- a/sysdeps/unix/sysv/linux/bits/fcntl-linux.h
> +++ b/sysdeps/unix/sysv/linux/bits/fcntl-linux.h
> @@ -101,7 +101,7 @@
>  #endif
>  
>  #ifndef F_GETLK
> -# ifndef __USE_FILE_OFFSET64
> +# if !defined(__USE_FILE_OFFSET64) && __TIMESIZE != 64
>  #  define F_GETLK	5	/* Get record locking info.  */
>  #  define F_SETLK	6	/* Set record locking info (non-blocking).  */
>  #  define F_SETLKW	7	/* Set record locking info (blocking).  */

We have a similar construct in many installed headers.  Is this really
the only place that needs changing?

Thanks,
Florian

Re: [PATCH] io: Fix record locking contants on 32 bit arch with 64 bit default time_t (BZ#30477)

[Andreas Schwab]

On Mai 24 2023, Adhemerval Zanella via Libc-alpha wrote:

> diff --git a/sysdeps/unix/sysv/linux/bits/fcntl-linux.h b/sysdeps/unix/sysv/linux/bits/fcntl-linux.h
> index ca6a0d7516..3c15625599 100644
> --- a/sysdeps/unix/sysv/linux/bits/fcntl-linux.h
> +++ b/sysdeps/unix/sysv/linux/bits/fcntl-linux.h
> @@ -101,7 +101,7 @@
>  #endif
>  
>  #ifndef F_GETLK
> -# ifndef __USE_FILE_OFFSET64
> +# if !defined(__USE_FILE_OFFSET64) && __TIMESIZE != 64

The defined operator does not need parens.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] io: Fix record locking contants on 32 bit arch with 64 bit default time_t (BZ#30477)

[Adhemerval Zanella Netto]

On 25/05/23 05:48, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> diff --git a/sysdeps/unix/sysv/linux/bits/fcntl-linux.h b/sysdeps/unix/sysv/linux/bits/fcntl-linux.h
>> index ca6a0d7516..3c15625599 100644
>> --- a/sysdeps/unix/sysv/linux/bits/fcntl-linux.h
>> +++ b/sysdeps/unix/sysv/linux/bits/fcntl-linux.h
>> @@ -101,7 +101,7 @@
>>  #endif
>>  
>>  #ifndef F_GETLK
>> -# ifndef __USE_FILE_OFFSET64
>> +# if !defined(__USE_FILE_OFFSET64) && __TIMESIZE != 64
>>  #  define F_GETLK	5	/* Get record locking info.  */
>>  #  define F_SETLK	6	/* Set record locking info (non-blocking).  */
>>  #  define F_SETLKW	7	/* Set record locking info (blocking).  */
> 
> We have a similar construct in many installed headers.  Is this really
> the only place that needs changing?

I did a quick check on the installed headers:

/usr/include/aio.h                It conditionalizes the existence of a pad after aio_offset,
                                  and it will create a zero size array in this case.  Not ideal,
                                  but this is not strictly an issue. 
                                  It also defined whether to redirect to the LFS variants (which 
                                  are all aliases)


/usr/include/bits/struct_stat.h   It is handled by each architecture and the generic should
                                  be fixed by 7a6ca82f8007ddbd43e2b8fce806ba7101ee47f5.

/usr/include/fcntl.h
/usr/include/stdio.h 
/usr/include/sys/mman.h 
/usr/include/fcntl.h 
/usr/include/sys/types.h
/usr/include/sys/stat.h
/usr/include/dirent.h             Define types that change size for LFS, which should not matter
                                  since in this case they are essentially the same; and whether 
                                  to redirect to the LFS variants (which also should not matter
                                  since they are aliases).

/usr/include/bits/unistd.h
/usr/include/fts.h
/usr/include/ftw.h
/usr/include/sys/resource.h
/usr/include/sys/sendfile.h
/usr/include/sys/statfs.h
/usr/include/sys/statvfs.h
/usr/include/bits/fcntl-linux.h
/usr/include/bits/fcntl2.h
/usr/include/stdlib.h
/usr/include/glob.h               Define whether to redirect to the LFS variants (which are
                                  all aliases).

So it does seems that this is the missing spot of kernel interface that is
changed whether LFS is used or not.