Re: [musl] Fix the return value of pthread_getschedparam in musl libc

["dalias@aerifal.cx" <dalias@aerifal.cx>]

On Wed, May 20, 2020 at 02:09:25AM +0000, tangyizhou wrote:
> Hi,
> 
> >From the latest upstream, I find the return value of
> >pthread_getschedparam() is wrong when syscall sched_getparam
> >succeeds but sched_getscheduler fails. It is rare to see this
> >situation. When it happens, pthread_getschedparam() will return r
> >which must be 0, that means a success according to POSIX 2017
> >specification. It's wrong.
> 
> Also, POSIX 2017 specification says an error number shall be
> returned to indicate the error. I think returning -*policy is
> appropriate because syscall sched_getscheduler returns an negative
> error code in kernel like Linux when it fails.

What is the situation under which SYS_sched_getparam can succeed but
SYS_sched_getscheduler can fail? The Linux man page documenting the
syscall:

http://man7.org/linux/man-pages/man2/sched_setscheduler.2.html

does not specify any errors meaningful for SYS_sched_getscheduler
other than ESRCH which is already precluded by the previous syscall
having succeeded.

> Here is my patch. I want to be Cc'd on replies. Thanks.

I've tried to CC @huawei.com addresses before but your mail system is
misconfigured to treat SPF neutral as fail. I don't know if this is
something you can get fixed, but getting it fixed would make it easier
to participate in open source projects. Trying again now anyway in
case it's already fixed.

> diff --git a/src/thread/pthread_getschedparam.c b/src/thread/pthread_getschedparam.c
> index 1cba073d..8203d609 100644
> --- a/src/thread/pthread_getschedparam.c
> +++ b/src/thread/pthread_getschedparam.c
> @@ -12,6 +12,9 @@ int pthread_getschedparam(pthread_t t, int *restrict policy, struct sched_param
>                 if (!r) {
>                         *policy = __syscall(SYS_sched_getscheduler, t->tid);
>                 }
> +               if (*policy < 0) {
> +                       r = -*policy;
> +               }
>         }
>         UNLOCK(t->killlock);
>         return r;

If there is a good reason to consider the possibility of
SYS_sched_getscheduler failing here, the patch has at least one bug
and at least one minor conformance issue. As written it examines
*policy even if nonzero r from SYS_sched_getparam failing caused
nothing to be written to *policy (in which case it's accessing
potentially-uninitialized memory, and clobbering the meaningful
error code r with a meaningless one from uninitialized memory). The
test would need to be inside the "if (!r) {" block to be valid.

Also (minor conformance/policy issue) if the function is going to fail
it should not write to *policy at all, but to a temp var, and only
copy to *policy on success.

Rich