[musl] [PATCH] add check for pthread_attr_setschedparam().

[musl] [PATCH] add check for pthread_attr_setschedparam().

[yan.li.cn]

From: Yan Li <yan.li.cn@windriver.com>

---
 src/thread/pthread_attr_setschedparam.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
 mode change 100644 => 100755 src/thread/pthread_attr_setschedparam.c

diff --git a/src/thread/pthread_attr_setschedparam.c b/src/thread/pthread_attr_setschedparam.c
old mode 100644
new mode 100755
index d4c1204f..4e28415e
--- a/src/thread/pthread_attr_setschedparam.c
+++ b/src/thread/pthread_attr_setschedparam.c
@@ -2,6 +2,13 @@
 
 int pthread_attr_setschedparam(pthread_attr_t *restrict a, const struct sched_param *restrict param)
 {
-	a->_a_prio = param->sched_priority;
-	return 0;
+    int min = sched_get_priority_min (a->_a_policy);
+    int max = sched_get_priority_max (a->_a_policy);
+
+    if (min < 0 || max < 0 || param->sched_priority < min || param->sched_priority > max){
+        return EINVAL;
+    }
+
+    a->_a_prio = param->sched_priority;
+    return 0;
 }
-- 
2.34.1

Re: [musl] [PATCH] add check for pthread_attr_setschedparam().

[Rich Felker]

On Tue, Dec 24, 2024 at 10:03:01AM +0800, yan.li.cn@windriver.com wrote:
> From: Yan Li <yan.li.cn@windriver.com>
> 
> ---
>  src/thread/pthread_attr_setschedparam.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>  mode change 100644 => 100755 src/thread/pthread_attr_setschedparam.c
> 
> diff --git a/src/thread/pthread_attr_setschedparam.c b/src/thread/pthread_attr_setschedparam.c
> old mode 100644
> new mode 100755
> index d4c1204f..4e28415e
> --- a/src/thread/pthread_attr_setschedparam.c
> +++ b/src/thread/pthread_attr_setschedparam.c
> @@ -2,6 +2,13 @@
>  
>  int pthread_attr_setschedparam(pthread_attr_t *restrict a, const struct sched_param *restrict param)
>  {
> -	a->_a_prio = param->sched_priority;
> -	return 0;
> +    int min = sched_get_priority_min (a->_a_policy);
> +    int max = sched_get_priority_max (a->_a_policy);
> +
> +    if (min < 0 || max < 0 || param->sched_priority < min || param->sched_priority > max){
> +        return EINVAL;
> +    }
> +
> +    a->_a_prio = param->sched_priority;
> +    return 0;
>  }
> -- 
> 2.34.1

I don't see a good motivation for this change. The error conditions
are MAY FAIL, not SHALL FAIL, and checking for them incurs two useless
syscalls on every call to a function that's otherwise syscall-free.

Rich

Re: [musl] [PATCH] add check for pthread_attr_setschedparam().

[Markus Wichmann]

Am Tue, Dec 24, 2024 at 04:41:04AM -0500 schrieb Rich Felker:
> On Tue, Dec 24, 2024 at 10:03:01AM +0800, yan.li.cn@windriver.com wrote:
> >  int pthread_attr_setschedparam(pthread_attr_t *restrict a, const struct sched_param *restrict param)
> >  {
> > -	a->_a_prio = param->sched_priority;
> > -	return 0;
> > +    int min = sched_get_priority_min (a->_a_policy);
> > +    int max = sched_get_priority_max (a->_a_policy);
> > +
> > +    if (min < 0 || max < 0 || param->sched_priority < min || param->sched_priority > max){
> > +        return EINVAL;
> > +    }
> > +
> > +    a->_a_prio = param->sched_priority;
> > +    return 0;
> >  }
> > --
> > 2.34.1
>
> I don't see a good motivation for this change. The error conditions
> are MAY FAIL, not SHALL FAIL, and checking for them incurs two useless
> syscalls on every call to a function that's otherwise syscall-free.
>
> Rich

It is also technically wrong. You are not supposed to relate different
attributes until the attributes actually get used.  Intermediate results
are allowed to be invalid. That's just how the entire POSIX *attr* API
works.

In this case, a scheduling priority may be wrong for the currently
selected policy, but you don't know what policy is used in the
pthread_create() call, or if the flags even contain
PTHREAD_EXPLICIT_SCHED at that point. The checking can only happen in
pthread_create(), and it doesn't even have to check these things
explicitly, it can just delegate that to the kernel when it sets the
scheduling attributes of the new thread.

Ciao,
Markus