[PATCH] Preserve select() behavior on arm64

[PATCH] Preserve select() behavior on arm64

[Peter Hurley]

On Linux, all the select-related syscalls update the timeout value to
indicate how much elapsed time the syscall consumed, and many programs
expect this behavior when running on Linux.

Newer archs like arm64 have deprecated the select syscall because the
pselect syscall is a superset implementation of the select syscall.

A complication of implementing select() with the pselect syscall is
that the timeouts are specified in different units; select() accepts
a struct timeval ptr whereas the pselect syscall takes a struct
timespec ptr. These are trivial convertible; struct timeval is
specified in seconds + microseconds and struct timespec is in
seconds + nanoseconds.

For kernel configurations without select syscall available, update the
caller's struct timeval argument after the pselect syscall.
---
 src/select/select.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/src/select/select.c b/src/select/select.c
index 7b5f6dcf7a53..45d4cb7a3d0a 100644
--- a/src/select/select.c
+++ b/src/select/select.c
@@ -12,15 +12,16 @@ int select(int n, fd_set *restrict rfds, fd_set *restrict wfds, fd_set *restrict
 #else
 	syscall_arg_t data[2] = { 0, _NSIG/8 };
 	struct timespec ts;
+	int result;
 	if (tv) {
-		if (tv->tv_sec < 0 || tv->tv_usec < 0)
-			return __syscall_ret(-EINVAL);
-		time_t extra_secs = tv->tv_usec / 1000000;
-		ts.tv_nsec = tv->tv_usec % 1000000 * 1000;
-		const time_t max_time = (1ULL<<8*sizeof(time_t)-1)-1;
-		ts.tv_sec = extra_secs > max_time - tv->tv_sec ?
-			max_time : tv->tv_sec + extra_secs;
+		ts->tv_sec = tv->tv_sec;
+		ts->tv_nsec = tv->usec * 1000;
 	}
-	return syscall_cp(SYS_pselect6, n, rfds, wfds, efds, tv ? &ts : 0, data);
+	result = syscall_cp(SYS_pselect6, n, rfds, wfds, efds, tv ? &ts : 0, data);
+	if (tv) {
+		tv->tv_sec = ts->tv_sec;
+		tv->tv_usec = ts->tv_nsec / 1000;
+	}
+	return result;
 #endif
 }
-- 
2.14.2

Re: [PATCH] Preserve select() behavior on arm64

[Peter Hurley]

Ping?

On Fri, May 25, 2018 at 1:52 PM, Peter Hurley <peter@meraki.com> wrote:
> On Linux, all the select-related syscalls update the timeout value to
> indicate how much elapsed time the syscall consumed, and many programs
> expect this behavior when running on Linux.
>
> Newer archs like arm64 have deprecated the select syscall because the
> pselect syscall is a superset implementation of the select syscall.
>
> A complication of implementing select() with the pselect syscall is
> that the timeouts are specified in different units; select() accepts
> a struct timeval ptr whereas the pselect syscall takes a struct
> timespec ptr. These are trivial convertible; struct timeval is
> specified in seconds + microseconds and struct timespec is in
> seconds + nanoseconds.
>
> For kernel configurations without select syscall available, update the
> caller's struct timeval argument after the pselect syscall.
> ---
>  src/select/select.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/src/select/select.c b/src/select/select.c
> index 7b5f6dcf7a53..45d4cb7a3d0a 100644
> --- a/src/select/select.c
> +++ b/src/select/select.c
> @@ -12,15 +12,16 @@ int select(int n, fd_set *restrict rfds, fd_set *restrict wfds, fd_set *restrict
>  #else
>         syscall_arg_t data[2] = { 0, _NSIG/8 };
>         struct timespec ts;
> +       int result;
>         if (tv) {
> -               if (tv->tv_sec < 0 || tv->tv_usec < 0)
> -                       return __syscall_ret(-EINVAL);
> -               time_t extra_secs = tv->tv_usec / 1000000;
> -               ts.tv_nsec = tv->tv_usec % 1000000 * 1000;
> -               const time_t max_time = (1ULL<<8*sizeof(time_t)-1)-1;
> -               ts.tv_sec = extra_secs > max_time - tv->tv_sec ?
> -                       max_time : tv->tv_sec + extra_secs;
> +               ts->tv_sec = tv->tv_sec;
> +               ts->tv_nsec = tv->usec * 1000;
>         }
> -       return syscall_cp(SYS_pselect6, n, rfds, wfds, efds, tv ? &ts : 0, data);
> +       result = syscall_cp(SYS_pselect6, n, rfds, wfds, efds, tv ? &ts : 0, data);
> +       if (tv) {
> +               tv->tv_sec = ts->tv_sec;
> +               tv->tv_usec = ts->tv_nsec / 1000;
> +       }
> +       return result;
>  #endif
>  }
> --
> 2.14.2
>

Re: [PATCH] Preserve select() behavior on arm64

[Rich Felker]

On Fri, May 25, 2018 at 01:52:40PM -0700, Peter Hurley wrote:
> On Linux, all the select-related syscalls update the timeout value to
> indicate how much elapsed time the syscall consumed, and many programs
> expect this behavior when running on Linux.
> 
> Newer archs like arm64 have deprecated the select syscall because the
> pselect syscall is a superset implementation of the select syscall.
> 
> A complication of implementing select() with the pselect syscall is
> that the timeouts are specified in different units; select() accepts
> a struct timeval ptr whereas the pselect syscall takes a struct
> timespec ptr. These are trivial convertible; struct timeval is
> specified in seconds + microseconds and struct timespec is in
> seconds + nanoseconds.
> 
> For kernel configurations without select syscall available, update the
> caller's struct timeval argument after the pselect syscall.
> ---
>  src/select/select.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/src/select/select.c b/src/select/select.c
> index 7b5f6dcf7a53..45d4cb7a3d0a 100644
> --- a/src/select/select.c
> +++ b/src/select/select.c
> @@ -12,15 +12,16 @@ int select(int n, fd_set *restrict rfds, fd_set *restrict wfds, fd_set *restrict
>  #else
>  	syscall_arg_t data[2] = { 0, _NSIG/8 };
>  	struct timespec ts;
> +	int result;
>  	if (tv) {
> -		if (tv->tv_sec < 0 || tv->tv_usec < 0)
> -			return __syscall_ret(-EINVAL);
> -		time_t extra_secs = tv->tv_usec / 1000000;
> -		ts.tv_nsec = tv->tv_usec % 1000000 * 1000;
> -		const time_t max_time = (1ULL<<8*sizeof(time_t)-1)-1;
> -		ts.tv_sec = extra_secs > max_time - tv->tv_sec ?
> -			max_time : tv->tv_sec + extra_secs;
> +		ts->tv_sec = tv->tv_sec;
> +		ts->tv_nsec = tv->usec * 1000;
>  	}
> -	return syscall_cp(SYS_pselect6, n, rfds, wfds, efds, tv ? &ts : 0, data);
> +	result = syscall_cp(SYS_pselect6, n, rfds, wfds, efds, tv ? &ts : 0, data);
> +	if (tv) {
> +		tv->tv_sec = ts->tv_sec;
> +		tv->tv_usec = ts->tv_nsec / 1000;
> +	}
> +	return result;
>  #endif
>  }
> -- 
> 2.14.2

Sorry for not replying to this sooner. I was unsure what to say at
first and then it just slipped my mind to go back to it.

Normally we don't aim to duplicate behavior that is explicitly
documented as non-portable, because programs depending on it should be
fixed (they're already broken on many non-linux systems).

Where it varies by arch because of implementation details like the
above (whether there's a SYS_select or SYS_pselect has to be used)
maybe the principle of consistent behavior across archs has something
to say here.

I'd almost rather do it the opposite (modern/pselect-like) direction,
making a copy of the struct to pass to SYS_select so the caller's copy
doesn't get clobbered, but maybe you like that even less..

Thoughts from anyone else?

Rich

Re: [PATCH] Preserve select() behavior on arm64

[Rich Felker]

On Wed, Aug 29, 2018 at 06:42:47PM -0400, Rich Felker wrote:
> On Fri, May 25, 2018 at 01:52:40PM -0700, Peter Hurley wrote:
> > On Linux, all the select-related syscalls update the timeout value to
> > indicate how much elapsed time the syscall consumed, and many programs
> > expect this behavior when running on Linux.
> > 
> > Newer archs like arm64 have deprecated the select syscall because the
> > pselect syscall is a superset implementation of the select syscall.
> > 
> > A complication of implementing select() with the pselect syscall is
> > that the timeouts are specified in different units; select() accepts
> > a struct timeval ptr whereas the pselect syscall takes a struct
> > timespec ptr. These are trivial convertible; struct timeval is
> > specified in seconds + microseconds and struct timespec is in
> > seconds + nanoseconds.
> > 
> > For kernel configurations without select syscall available, update the
> > caller's struct timeval argument after the pselect syscall.
> > ---
> >  src/select/select.c | 17 +++++++++--------
> >  1 file changed, 9 insertions(+), 8 deletions(-)
> > 
> > diff --git a/src/select/select.c b/src/select/select.c
> > index 7b5f6dcf7a53..45d4cb7a3d0a 100644
> > --- a/src/select/select.c
> > +++ b/src/select/select.c
> > @@ -12,15 +12,16 @@ int select(int n, fd_set *restrict rfds, fd_set *restrict wfds, fd_set *restrict
> >  #else
> >  	syscall_arg_t data[2] = { 0, _NSIG/8 };
> >  	struct timespec ts;
> > +	int result;
> >  	if (tv) {
> > -		if (tv->tv_sec < 0 || tv->tv_usec < 0)
> > -			return __syscall_ret(-EINVAL);
> > -		time_t extra_secs = tv->tv_usec / 1000000;
> > -		ts.tv_nsec = tv->tv_usec % 1000000 * 1000;
> > -		const time_t max_time = (1ULL<<8*sizeof(time_t)-1)-1;
> > -		ts.tv_sec = extra_secs > max_time - tv->tv_sec ?
> > -			max_time : tv->tv_sec + extra_secs;
> > +		ts->tv_sec = tv->tv_sec;
> > +		ts->tv_nsec = tv->usec * 1000;
> >  	}
> > -	return syscall_cp(SYS_pselect6, n, rfds, wfds, efds, tv ? &ts : 0, data);
> > +	result = syscall_cp(SYS_pselect6, n, rfds, wfds, efds, tv ? &ts : 0, data);
> > +	if (tv) {
> > +		tv->tv_sec = ts->tv_sec;
> > +		tv->tv_usec = ts->tv_nsec / 1000;
> > +	}
> > +	return result;
> >  #endif
> >  }
> > -- 
> > 2.14.2
> 
> Sorry for not replying to this sooner. I was unsure what to say at
> first and then it just slipped my mind to go back to it.

I think the patch is also incorrect as written. It removes the code
for handling timeval values which would be out of range if naively
converted to timespec (either<0 or usec>999999, and overflows). I'm
not sure how SYS_select handles updating the remaining time in the
overflow cases, so if you're trying to duplicate it, researching that
is probably called for. It may be right to just normalize it like
you're doing when converting back, but this will effectively report
the wrong amount of elapsed time if tv_sec+tv_usec/1000000 overflows.

Rich

Re: [PATCH] Preserve select() behavior on arm64

[Peter Hurley]

On Wed, Aug 29, 2018 at 3:56 PM, Rich Felker <dalias@libc.org> wrote:
> On Wed, Aug 29, 2018 at 06:42:47PM -0400, Rich Felker wrote:
>> On Fri, May 25, 2018 at 01:52:40PM -0700, Peter Hurley wrote:
>> > On Linux, all the select-related syscalls update the timeout value to
>> > indicate how much elapsed time the syscall consumed, and many programs
>> > expect this behavior when running on Linux.
>> >
>> > Newer archs like arm64 have deprecated the select syscall because the
>> > pselect syscall is a superset implementation of the select syscall.
>> >
>> > A complication of implementing select() with the pselect syscall is
>> > that the timeouts are specified in different units; select() accepts
>> > a struct timeval ptr whereas the pselect syscall takes a struct
>> > timespec ptr. These are trivial convertible; struct timeval is
>> > specified in seconds + microseconds and struct timespec is in
>> > seconds + nanoseconds.
>> >
>> > For kernel configurations without select syscall available, update the
>> > caller's struct timeval argument after the pselect syscall.
>> > ---
>> >  src/select/select.c | 17 +++++++++--------
>> >  1 file changed, 9 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/src/select/select.c b/src/select/select.c
>> > index 7b5f6dcf7a53..45d4cb7a3d0a 100644
>> > --- a/src/select/select.c
>> > +++ b/src/select/select.c
>> > @@ -12,15 +12,16 @@ int select(int n, fd_set *restrict rfds, fd_set *restrict wfds, fd_set *restrict
>> >  #else
>> >     syscall_arg_t data[2] = { 0, _NSIG/8 };
>> >     struct timespec ts;
>> > +   int result;
>> >     if (tv) {
>> > -           if (tv->tv_sec < 0 || tv->tv_usec < 0)
>> > -                   return __syscall_ret(-EINVAL);
>> > -           time_t extra_secs = tv->tv_usec / 1000000;
>> > -           ts.tv_nsec = tv->tv_usec % 1000000 * 1000;
>> > -           const time_t max_time = (1ULL<<8*sizeof(time_t)-1)-1;
>> > -           ts.tv_sec = extra_secs > max_time - tv->tv_sec ?
>> > -                   max_time : tv->tv_sec + extra_secs;
>> > +           ts->tv_sec = tv->tv_sec;
>> > +           ts->tv_nsec = tv->usec * 1000;
>> >     }
>> > -   return syscall_cp(SYS_pselect6, n, rfds, wfds, efds, tv ? &ts : 0, data);
>> > +   result = syscall_cp(SYS_pselect6, n, rfds, wfds, efds, tv ? &ts : 0, data);
>> > +   if (tv) {
>> > +           tv->tv_sec = ts->tv_sec;
>> > +           tv->tv_usec = ts->tv_nsec / 1000;
>> > +   }
>> > +   return result;
>> >  #endif
>> >  }
>> > --
>> > 2.14.2
>>
>> Sorry for not replying to this sooner. I was unsure what to say at
>> first and then it just slipped my mind to go back to it.
>
> I think the patch is also incorrect as written. It removes the code
> for handling timeval values which would be out of range if naively
> converted to timespec (either<0 or usec>999999, and overflows). I'm
> not sure how SYS_select handles updating the remaining time in the
> overflow cases, so if you're trying to duplicate it, researching that
> is probably called for. It may be right to just normalize it like
> you're doing when converting back, but this will effectively report
> the wrong amount of elapsed time if tv_sec+tv_usec/1000000 overflows.
>
> Rich

Linux returns -EINVAL for not normalized timespec and timeval inputs.

Regards,
Peter Hurley

Re: [PATCH] Preserve select() behavior on arm64

[Peter Hurley]

On Wed, Aug 29, 2018 at 3:42 PM, Rich Felker <dalias@libc.org> wrote:
> On Fri, May 25, 2018 at 01:52:40PM -0700, Peter Hurley wrote:
>> On Linux, all the select-related syscalls update the timeout value to
>> indicate how much elapsed time the syscall consumed, and many programs
>> expect this behavior when running on Linux.
>>
>> Newer archs like arm64 have deprecated the select syscall because the
>> pselect syscall is a superset implementation of the select syscall.
>>
>> A complication of implementing select() with the pselect syscall is
>> that the timeouts are specified in different units; select() accepts
>> a struct timeval ptr whereas the pselect syscall takes a struct
>> timespec ptr. These are trivial convertible; struct timeval is
>> specified in seconds + microseconds and struct timespec is in
>> seconds + nanoseconds.
>>
>> For kernel configurations without select syscall available, update the
>> caller's struct timeval argument after the pselect syscall.
>> ---
>>  src/select/select.c | 17 +++++++++--------
>>  1 file changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/select/select.c b/src/select/select.c
>> index 7b5f6dcf7a53..45d4cb7a3d0a 100644
>> --- a/src/select/select.c
>> +++ b/src/select/select.c
>> @@ -12,15 +12,16 @@ int select(int n, fd_set *restrict rfds, fd_set *restrict wfds, fd_set *restrict
>>  #else
>>       syscall_arg_t data[2] = { 0, _NSIG/8 };
>>       struct timespec ts;
>> +     int result;
>>       if (tv) {
>> -             if (tv->tv_sec < 0 || tv->tv_usec < 0)
>> -                     return __syscall_ret(-EINVAL);
>> -             time_t extra_secs = tv->tv_usec / 1000000;
>> -             ts.tv_nsec = tv->tv_usec % 1000000 * 1000;
>> -             const time_t max_time = (1ULL<<8*sizeof(time_t)-1)-1;
>> -             ts.tv_sec = extra_secs > max_time - tv->tv_sec ?
>> -                     max_time : tv->tv_sec + extra_secs;
>> +             ts->tv_sec = tv->tv_sec;
>> +             ts->tv_nsec = tv->usec * 1000;
>>       }
>> -     return syscall_cp(SYS_pselect6, n, rfds, wfds, efds, tv ? &ts : 0, data);
>> +     result = syscall_cp(SYS_pselect6, n, rfds, wfds, efds, tv ? &ts : 0, data);
>> +     if (tv) {
>> +             tv->tv_sec = ts->tv_sec;
>> +             tv->tv_usec = ts->tv_nsec / 1000;
>> +     }
>> +     return result;
>>  #endif
>>  }
>> --
>> 2.14.2
>
> Sorry for not replying to this sooner. I was unsure what to say at
> first and then it just slipped my mind to go back to it.
>
> Normally we don't aim to duplicate behavior that is explicitly
> documented as non-portable, because programs depending on it should be
> fixed (they're already broken on many non-linux systems).
>
> Where it varies by arch because of implementation details like the
> above (whether there's a SYS_select or SYS_pselect has to be used)
> maybe the principle of consistent behavior across archs has something
> to say here.
>
> I'd almost rather do it the opposite (modern/pselect-like) direction,
> making a copy of the struct to pass to SYS_select so the caller's copy
> doesn't get clobbered, but maybe you like that even less..
>
> Thoughts from anyone else?
>
> Rich


Thanks for the reply.

With respect to non-portability, this is behavior that is specifically
allowed for select() in POSIX.
My goal with the patch is principle-of-least-surprise between arches
on musl, and libc on Linux.

Regards,
Peter Hurley