Re: [PATCH] Fix misreported errno on preadv2/pwritev2 (BZ#23579)

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

I will commit this patch shortly if no one opposes it.

On 20/09/2018 15:32, Adhemerval Zanella wrote:
> Ping.
> 
> On 29/08/2018 14:20, Adhemerval Zanella wrote:
>> The fallback code of Linux wrapper for preadv2/pwritev2 executes
>> regardless of the errno code for preadv2, instead of the case where
>> the syscall is not supported.
>>
>> This fixes it by calling the fallback code iff errno is ENOSYS. The
>> patch also adds tests for both invalid file descriptor and invalid
>> iov_len and vector count.
>>
>> The only discrepancy between preadv2 and fallback code regarding
>> error reporting is when an invalid flags are used.  The fallback code
>> bails out earlier with ENOTSUP instead of EINVAL/EBADF when the syscall
>> is used.
>>
>> Checked on x86_64-linux-gnu on a 4.4.0 and 4.15.0 kernel.
>>
>> 	[BZ #23579]
>> 	* misc/tst-preadvwritev2-common.c (do_test_with_invalid_fd): New
>> 	test.
>> 	* misc/tst-preadvwritev2.c, misc/tst-preadvwritev64v2.c (do_test):
>> 	Call do_test_with_invalid_fd.
>> 	* sysdeps/unix/sysv/linux/preadv2.c (preadv2): Use fallback code iff
>> 	errno is ENOSYS.
>> 	* sysdeps/unix/sysv/linux/preadv64v2.c (preadv64v2): Likewise.
>> 	* sysdeps/unix/sysv/linux/pwritev2.c (pwritev2): Likewise.
>> 	* sysdeps/unix/sysv/linux/pwritev64v2.c (pwritev64v2): Likewise.
>> ---
>>  ChangeLog                             | 13 ++++++
>>  misc/tst-preadvwritev2-common.c       | 65 +++++++++++++++++++++++++--
>>  misc/tst-preadvwritev2.c              |  2 +
>>  misc/tst-preadvwritev64v2.c           |  2 +
>>  sysdeps/unix/sysv/linux/preadv2.c     |  2 +-
>>  sysdeps/unix/sysv/linux/preadv64v2.c  |  2 +-
>>  sysdeps/unix/sysv/linux/pwritev2.c    |  2 +-
>>  sysdeps/unix/sysv/linux/pwritev64v2.c |  2 +-
>>  8 files changed, 83 insertions(+), 7 deletions(-)
>>
>> diff --git a/misc/tst-preadvwritev2-common.c b/misc/tst-preadvwritev2-common.c
>> index f889a21544..50b9da3fea 100644
>> --- a/misc/tst-preadvwritev2-common.c
>> +++ b/misc/tst-preadvwritev2-common.c
>> @@ -19,9 +19,6 @@
>>  #include <limits.h>
>>  #include <support/check.h>
>>  
>> -static void
>> -do_test_with_invalid_flags (void)
>> -{
>>  #ifndef RWF_HIPRI
>>  # define RWF_HIPRI 0
>>  #endif
>> @@ -39,6 +36,68 @@ do_test_with_invalid_flags (void)
>>  #endif
>>  #define RWF_SUPPORTED	(RWF_HIPRI | RWF_DSYNC | RWF_SYNC | RWF_NOWAIT \
>>  			 | RWF_APPEND)
>> +
>> +static void
>> +do_test_with_invalid_fd (void)
>> +{
>> +  char buf[256];
>> +  struct iovec iov = { buf, sizeof buf };
>> +
>> +  /* Check with flag being 0 to use the fallback code which calls pwritev
>> +     or writev.  */
>> +  TEST_VERIFY (preadv2 (-1, &iov, 1, -1, 0) == -1);
>> +  TEST_COMPARE (errno, EBADF);
>> +  TEST_VERIFY (pwritev2 (-1, &iov, 1, -1, 0) == -1);
>> +  TEST_COMPARE (errno, EBADF);
>> +
>> +  /* Same tests as before but with flags being different than 0.  Since
>> +     there is no emulation for any flag value, fallback code returns
>> +     ENOTSUP.  This is different running on a kernel with preadv2/pwritev2
>> +     support, where EBADF is returned).  */
>> +  TEST_VERIFY (preadv2 (-1, &iov, 1, 0, RWF_HIPRI) == -1);
>> +  TEST_VERIFY (errno == EBADF || errno == ENOTSUP);
>> +  TEST_VERIFY (pwritev2 (-1, &iov, 1, 0, RWF_HIPRI) == -1);
>> +  TEST_VERIFY (errno == EBADF || errno == ENOTSUP);
>> +}
>> +
>> +static void
>> +do_test_with_invalid_iov (void)
>> +{
>> +  {
>> +    char buf[256];
>> +    struct iovec iov;
>> +
>> +    iov.iov_base = buf;
>> +    iov.iov_len = (size_t)SSIZE_MAX + 1;
>> +
>> +    TEST_VERIFY (preadv2 (temp_fd, &iov, 1, 0, 0) == -1);
>> +    TEST_COMPARE (errno, EINVAL);
>> +    TEST_VERIFY (pwritev2 (temp_fd, &iov, 1, 0, 0) == -1);
>> +    TEST_COMPARE (errno, EINVAL);
>> +
>> +    /* Same as for invalid file descriptor tests, emulation fallback
>> +       first checks for flag value and return ENOTSUP.  */
>> +    TEST_VERIFY (preadv2 (temp_fd, &iov, 1, 0, RWF_HIPRI) == -1);
>> +    TEST_VERIFY (errno == EINVAL || errno == ENOTSUP);
>> +    TEST_VERIFY (pwritev2 (temp_fd, &iov, 1, 0, RWF_HIPRI) == -1);
>> +    TEST_VERIFY (errno == EINVAL || errno == ENOTSUP);
>> +  }
>> +
>> +  {
>> +    /* An invalid iovec buffer should trigger an invalid memory access
>> +       or an error (Linux for instance returns EFAULT).  */
>> +    struct iovec iov[IOV_MAX+1] = { 0 };
>> +
>> +    TEST_VERIFY (preadv2 (temp_fd, iov, IOV_MAX + 1, 0, RWF_HIPRI) == -1);
>> +    TEST_VERIFY (errno == EINVAL || errno == ENOTSUP);
>> +    TEST_VERIFY (pwritev2 (temp_fd, iov, IOV_MAX + 1, 0, RWF_HIPRI) == -1);
>> +    TEST_VERIFY (errno == EINVAL || errno == ENOTSUP);
>> +  }
>> +}
>> +
>> +static void
>> +do_test_with_invalid_flags (void)
>> +{
>>    /* Set the next bit from the mask of all supported flags.  */
>>    int invalid_flag = RWF_SUPPORTED != 0 ? __builtin_clz (RWF_SUPPORTED) : 2;
>>    invalid_flag = 0x1 << ((sizeof (int) * CHAR_BIT) - invalid_flag);
>> diff --git a/misc/tst-preadvwritev2.c b/misc/tst-preadvwritev2.c
>> index be22802dbe..cb58cbe41e 100644
>> --- a/misc/tst-preadvwritev2.c
>> +++ b/misc/tst-preadvwritev2.c
>> @@ -30,6 +30,8 @@ do_test (void)
>>  {
>>    do_test_with_invalid_flags ();
>>    do_test_without_offset ();
>> +  do_test_with_invalid_fd ();
>> +  do_test_with_invalid_iov ();
>>  
>>    return do_test_with_offset (0);
>>  }
>> diff --git a/misc/tst-preadvwritev64v2.c b/misc/tst-preadvwritev64v2.c
>> index 8d3cc32b28..6a9de54c78 100644
>> --- a/misc/tst-preadvwritev64v2.c
>> +++ b/misc/tst-preadvwritev64v2.c
>> @@ -32,6 +32,8 @@ do_test (void)
>>  {
>>    do_test_with_invalid_flags ();
>>    do_test_without_offset ();
>> +  do_test_with_invalid_fd ();
>> +  do_test_with_invalid_iov ();
>>  
>>    return do_test_with_offset (0);
>>  }
>> diff --git a/sysdeps/unix/sysv/linux/preadv2.c b/sysdeps/unix/sysv/linux/preadv2.c
>> index c8bf0764ef..bb08cbc5fd 100644
>> --- a/sysdeps/unix/sysv/linux/preadv2.c
>> +++ b/sysdeps/unix/sysv/linux/preadv2.c
>> @@ -32,7 +32,7 @@ preadv2 (int fd, const struct iovec *vector, int count, off_t offset,
>>  # ifdef __NR_preadv2
>>    ssize_t result = SYSCALL_CANCEL (preadv2, fd, vector, count,
>>  				   LO_HI_LONG (offset), flags);
>> -  if (result >= 0)
>> +  if (result >= 0 || errno != ENOSYS)
>>      return result;
>>  # endif
>>    /* Trying to emulate the preadv2 syscall flags is troublesome:
>> diff --git a/sysdeps/unix/sysv/linux/preadv64v2.c b/sysdeps/unix/sysv/linux/preadv64v2.c
>> index d7400a0252..b72a047347 100644
>> --- a/sysdeps/unix/sysv/linux/preadv64v2.c
>> +++ b/sysdeps/unix/sysv/linux/preadv64v2.c
>> @@ -30,7 +30,7 @@ preadv64v2 (int fd, const struct iovec *vector, int count, off64_t offset,
>>  #ifdef __NR_preadv64v2
>>    ssize_t result = SYSCALL_CANCEL (preadv64v2, fd, vector, count,
>>  				   LO_HI_LONG (offset), flags);
>> -  if (result >= 0)
>> +  if (result >= 0 || errno != ENOSYS)
>>      return result;
>>  #endif
>>    /* Trying to emulate the preadv2 syscall flags is troublesome:
>> diff --git a/sysdeps/unix/sysv/linux/pwritev2.c b/sysdeps/unix/sysv/linux/pwritev2.c
>> index 29c2264c8f..26333ebd43 100644
>> --- a/sysdeps/unix/sysv/linux/pwritev2.c
>> +++ b/sysdeps/unix/sysv/linux/pwritev2.c
>> @@ -28,7 +28,7 @@ pwritev2 (int fd, const struct iovec *vector, int count, off_t offset,
>>  # ifdef __NR_pwritev2
>>    ssize_t result = SYSCALL_CANCEL (pwritev2, fd, vector, count,
>>  				   LO_HI_LONG (offset), flags);
>> -  if (result >= 0)
>> +  if (result >= 0 || errno != ENOSYS)
>>      return result;
>>  # endif
>>    /* Trying to emulate the pwritev2 syscall flags is troublesome:
>> diff --git a/sysdeps/unix/sysv/linux/pwritev64v2.c b/sysdeps/unix/sysv/linux/pwritev64v2.c
>> index 42da321149..17ea905aa6 100644
>> --- a/sysdeps/unix/sysv/linux/pwritev64v2.c
>> +++ b/sysdeps/unix/sysv/linux/pwritev64v2.c
>> @@ -30,7 +30,7 @@ pwritev64v2 (int fd, const struct iovec *vector, int count, off64_t offset,
>>  #ifdef __NR_pwritev64v2
>>    ssize_t result = SYSCALL_CANCEL (pwritev64v2, fd, vector, count,
>>  				   LO_HI_LONG (offset), flags);
>> -  if (result >= 0)
>> +  if (result >= 0 || errno != ENOSYS)
>>      return result;
>>  #endif
>>    /* Trying to emulate the pwritev2 syscall flags is troublesome:
>>