[PATCH] nptl: Fix SYSCALL_CANCEL for return values larger than INT_MAX (BZ 33245)

[Sam James]

Sam James <sam@gentoo.org> writes:

> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
>
>> The SYSCALL_CANCEL calls __syscall_cancel, which in turn
>> calls __internal_syscall_cancel with an 'int' return instead of the
>> expected 'long int'.  This causes issues with syscalls that return
>> values larger than INT_MAX, such as copy_file_range [1].
>>
>> Checked on x86_64-linux-gnu.
>>
>> [1] https://debbugs.gnu.org/cgi/bugreport.cgi?bug=79139
>
> The patch is OK:
>
> Reviewed-by: Sam James <sam@gentoo.org>
>
>> ---
>>  nptl/cancellation.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> but I have a question about some other instances, though I didn't look
> afresh yet today, just from last night.
>
> Do we need something like the attached? I think the readlink one doesn't
> actually matter because of path sizes.

OK, looked more closely..

>
> diff --git a/sysdeps/unix/sysv/linux/dl-origin.c b/sysdeps/unix/sysv/linux/dl-origin.c
> index 3c52ba51a6..b7fec7cc3f 100644
> --- a/sysdeps/unix/sysv/linux/dl-origin.c
> +++ b/sysdeps/unix/sysv/linux/dl-origin.c
> @@ -32,7 +32,7 @@ _dl_get_origin (void)
>  {
>    char linkval[PATH_MAX];
>    char *result;
> -  int len;
> +  long len;
>  
>    len = INTERNAL_SYSCALL_CALL (readlinkat, AT_FDCWD, "/proc/self/exe",
>  			       linkval, sizeof (linkval));
> @@ -84,7 +84,7 @@ _dl_canonicalize (int fd)
>    struct fd_to_filename fdfilename;
>    char canonical[PATH_MAX];
>    char *path = __fd_to_filename (fd, &fdfilename);
> -  int size = INTERNAL_SYSCALL_CALL (readlinkat, AT_FDCWD, path,
> +  long size = INTERNAL_SYSCALL_CALL (readlinkat, AT_FDCWD, path,
>                                      canonical, PATH_MAX - 1);

Again, doesn't really matter in practice AFAICT.

>  
>    /* Check if the path was truncated.  */
> diff --git a/sysdeps/unix/sysv/linux/lseek.c b/sysdeps/unix/sysv/linux/lseek.c
> index ff83735d09..9a1383f782 100644
> --- a/sysdeps/unix/sysv/linux/lseek.c
> +++ b/sysdeps/unix/sysv/linux/lseek.c
> @@ -47,7 +47,7 @@ __lseek (int fd, off_t offset, int whence)
>  
>  # ifdef __NR__llseek
>    loff_t res;
> -  int rc = INLINE_SYSCALL_CALL (_llseek, fd,
> +  long int rc = INLINE_SYSCALL_CALL (_llseek, fd,
>  				(long) (((uint64_t) (offset)) >> 32),
>  				(long) offset, &res, whence);
>    return rc ?: lseek_overflow (res);

*llseek* uses a parameter for its return value, so it should be fine. If
the arch lacks llseek, the other path is fine.

> diff --git a/sysdeps/unix/sysv/linux/lseek64.c b/sysdeps/unix/sysv/linux/lseek64.c
> index 49f4136844..b5624388cc 100644
> --- a/sysdeps/unix/sysv/linux/lseek64.c
> +++ b/sysdeps/unix/sysv/linux/lseek64.c
> @@ -32,7 +32,7 @@ __lseek64 (int fd, off64_t offset, int whence)
>  
>  #ifdef __NR__llseek
>    loff_t res;
> -  int rc = INLINE_SYSCALL_CALL (_llseek, fd,
> +  long int rc = INLINE_SYSCALL_CALL (_llseek, fd,
>  				(long) (((uint64_t) (offset)) >> 32),
>  				(long) offset, &res, whence);

Ditto llseek/lseek above.

>    return rc ?: res;
> diff --git a/sysdeps/unix/sysv/linux/sendfile.c b/sysdeps/unix/sysv/linux/sendfile.c
> index 990a26a3ea..1f04ce5aab 100644
> --- a/sysdeps/unix/sysv/linux/sendfile.c
> +++ b/sysdeps/unix/sysv/linux/sendfile.c
> @@ -32,7 +32,7 @@ sendfile (int out_fd, int in_fd, off_t *offset, size_t count)
>    return INLINE_SYSCALL_CALL (sendfile, out_fd, in_fd, offset, count);
>  # else
>    __off64_t off64;
> -  int rc;
> +  ssize_t rc;
>  
>    if (offset != NULL)
>      {
>

This one looks wrong but all arches have __NR_sendfile, so limited impact.

>
> (Completely untested, it's just from the audit I did earlier.)
>
>> [...]
>
> sam

sam