[v4,1/6] Fix calling fcntl64 (fd, F_SETLK, &flock64)

  F_GETLK, F_SETLK, F_SETLKW must always be used with the regular struct
flock, not struct flock64, even if the function being called is fcntl64.
There are separate commands, F_GETLK64, F_SETLK64, and F_SETLKW64, that
are used with struct flock64. This is in contrast with the OFD locking
commands (F_OFD_GETLK, F_OFD_SETLK, F_OFD_SETLKW) that must be used with
struct flock in fcntl calls, and with struct flock64 in fcntl64 calls.

These mistakes were spotted by enabling the fcntl fortification that is
added in a following commit.

Fixes: 61d3db428176d9d0822e4e680305fe34285edff2
"login: pututxline could fail to overwrite existing entries [BZ #24902]"
Fixes: f6233ab412c3bebebacf65745e775e01506dd58d
"Linux: Add io/tst-o_path-locks test"

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---

Please tell me if I'm misunderstanding this!

 login/tst-pututxline-lockfail.c            | 2 +-
 sysdeps/unix/sysv/linux/tst-o_path-locks.c | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)
  

On 30/07/23 16:25, Sergey Bugaev via Libc-alpha wrote:
> F_GETLK, F_SETLK, F_SETLKW must always be used with the regular struct
> flock, not struct flock64, even if the function being called is fcntl64.
> There are separate commands, F_GETLK64, F_SETLK64, and F_SETLKW64, that
> are used with struct flock64. This is in contrast with the OFD locking
> commands (F_OFD_GETLK, F_OFD_SETLK, F_OFD_SETLKW) that must be used with
> struct flock in fcntl calls, and with struct flock64 in fcntl64 calls.
> 
> These mistakes were spotted by enabling the fcntl fortification that is
> added in a following commit.
> 
> Fixes: 61d3db428176d9d0822e4e680305fe34285edff2
> "login: pututxline could fail to overwrite existing entries [BZ #24902]"
> Fixes: f6233ab412c3bebebacf65745e775e01506dd58d
> "Linux: Add io/tst-o_path-locks test"
> 
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>

Nice, it seems that on both tests the fields that might trigger wrong values
by the kernel reading the struct with a wrong size (l_start and l_len) are 
always being 0 and thus it should not matter.

LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
> 
> Please tell me if I'm misunderstanding this!
> 
>  login/tst-pututxline-lockfail.c            | 2 +-
>  sysdeps/unix/sysv/linux/tst-o_path-locks.c | 6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/login/tst-pututxline-lockfail.c b/login/tst-pututxline-lockfail.c
> index 214d1106..ca82ba42 100644
> --- a/login/tst-pututxline-lockfail.c
> +++ b/login/tst-pututxline-lockfail.c
> @@ -77,7 +77,7 @@ subprocess_lock_file (void)
>       .l_type = F_RDLCK,
>       fl.l_whence = SEEK_SET,
>      };
> -  TEST_COMPARE (fcntl64 (fd, F_SETLKW, &fl), 0);
> +  TEST_COMPARE (fcntl64 (fd, F_SETLKW64, &fl), 0);
>  
>    /* Signal to the main process that the lock has been acquired.  */
>    xpthread_barrier_wait (barrier);
> diff --git a/sysdeps/unix/sysv/linux/tst-o_path-locks.c b/sysdeps/unix/sysv/linux/tst-o_path-locks.c
> index 0036868d..c3d1c0dc 100644
> --- a/sysdeps/unix/sysv/linux/tst-o_path-locks.c
> +++ b/sysdeps/unix/sysv/linux/tst-o_path-locks.c
> @@ -39,7 +39,7 @@ subprocess (void *closure)
>  {
>    int fd = xopen (path, O_RDWR, 0);
>    struct flock64 lock = { .l_type = F_WRLCK, };
> -  int ret = fcntl64 (fd, F_SETLK, &lock);
> +  int ret = fcntl64 (fd, F_SETLK64, &lock);
>    if (ret == 0)
>      *shared_errno = 0;
>    else
> @@ -76,7 +76,7 @@ do_test (void)
>    TEST_VERIFY (!probe_lock ());
>  
>    struct flock64 lock = { .l_type = F_WRLCK, };
> -  TEST_COMPARE (fcntl64 (fd, F_SETLK, &lock), 0);
> +  TEST_COMPARE (fcntl64 (fd, F_SETLK64, &lock), 0);
>  
>    /* The lock has been acquired.  */
>    TEST_VERIFY (probe_lock ());
> @@ -87,7 +87,7 @@ do_test (void)
>    TEST_VERIFY (!probe_lock ());
>  
>    /* But not if it is an O_PATH descriptor.  */
> -  TEST_COMPARE (fcntl64 (fd, F_SETLK, &lock), 0);
> +  TEST_COMPARE (fcntl64 (fd, F_SETLK64, &lock), 0);
>    xclose (xopen (path, O_PATH, 0));
>    TEST_VERIFY (probe_lock ());
>
  

Hello,

On Mon, Jul 31, 2023 at 8:50 PM Adhemerval Zanella Netto
<adhemerval.zanella@linaro.org> wrote:
> Nice, it seems that on both tests the fields that might trigger wrong values
> by the kernel reading the struct with a wrong size (l_start and l_len) are
> always being 0 and thus it should not matter.
>
> LGTM, thanks.

But please note that this only really fortifies tests & other code
that uses the public fcntl/fcntl64 API, not callers of __libc_fcntl64,
so it's not a guarantee that there are no more cases of this. Though I
guess this applies equally to all the other fortifications.

Sergey