This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
[ping3][PATCH v3] Consolidate pthread_attr value validation
- From: Siddhesh Poyarekar <siddhesh at redhat dot com>
- To: Roland McGrath <roland at hack dot frob dot com>
- Cc: libc-alpha at sourceware dot org
- Date: Mon, 15 Apr 2013 14:24:35 +0530
- Subject: [ping3][PATCH v3] Consolidate pthread_attr value validation
- References: <20130318111716 dot GE3969 at spoyarek dot pnq dot redhat dot com> <20130318165441 dot 575E52C08A at topped-with-meat dot com> <20130319113803 dot GB25837 at spoyarek dot pnq dot redhat dot com> <20130319181311 dot CC6542C081 at topped-with-meat dot com> <CAAHN_R0Ky6SbMhzJq2YzsGpseaFt+tEq6K9ey_88yHVoPpsnpw at mail dot gmail dot com> <20130322182234 dot 58DF32C06C at topped-with-meat dot com> <20130326114828 dot GA15083 at spoyarek dot pnq dot redhat dot com> <20130401054736 dot GZ17540 at spoyarek dot pnq dot redhat dot com> <20130409055021 dot GF15689 at spoyarek dot pnq dot redhat dot com>
Ping!
On Tue, Apr 09, 2013 at 11:20:21AM +0530, Siddhesh Poyarekar wrote:
> Ping!
>
> On Mon, Apr 01, 2013 at 11:17:37AM +0530, Siddhesh Poyarekar wrote:
> > Ping!
> >
> >
> > On Tue, Mar 26, 2013 at 05:18:28PM +0530, Siddhesh Poyarekar wrote:
> > > On Fri, Mar 22, 2013 at 11:22:34AM -0700, Roland McGrath wrote:
> > > > The cases here are simply optimization choices. Those should be left to
> > > > the compiler. 'inline' is a hint to the compiler about optimization
> > > > choice, though from the GCC documentation it's not clear to me it really
> > > > makes any difference in practice at -O2, but clear that it doesn't make any
> > > > difference at -O3. Hence it's clear that 'inline' should never be used in
> > > > .c files. That was the context of the previous discussion but I don't
> > > > think we were clear about the distinction between cases in .c files and
> > > > cases in .h files.
> > >
> > > OK, thanks for the clarification. Here's v3 of the cleanup patch.
> > >
> > > Siddhesh
> > >
> > > * pthreadP.h (check_sched_policy_attr): New inline function.
> > > (check_sched_priority_attr): Likewise.
> > > (check_stacksize_attr): Likewise.
> > > (__kernel_cpumask_size, __determine_cpumask_size): Declare
> > > extern.
> > > (check_cpuset_attr): New inline function.
> > > * pthread_attr_setschedparam (__pthread_attr_setschedparam):
> > > Use check_sched_priority_attr.
> > > * pthread_attr_setschedpolicy.c
> > > (__pthread_attr_setschedpolicy): Use check_sched_policy_attr.
> > > * pthread_attr_setstack.c (__pthread_attr_setstack): Use
> > > check_stacksize_attr.
> > > * pthread_attr_setstacksize.c (__pthread_attr_setstacksize):
> > > Likewise.
> > > * sysdeps/unix/sysv/linux/pthread_attr_setaffinity.c
> > > (__pthread_attr_setaffinity_new): Use check_cpuset_attr.
> > >
> > > diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
> > > index 39e930c..a34a889 100644
> > > --- a/nptl/pthreadP.h
> > > +++ b/nptl/pthreadP.h
> > > @@ -31,7 +31,7 @@
> > > #include <pthread-functions.h>
> > > #include <atomic.h>
> > > #include <kernel-features.h>
> > > -
> > > +#include <errno.h>
> > >
> > > /* Atomic operations on TLS memory. */
> > > #ifndef THREAD_ATOMIC_CMPXCHG_VAL
> > > @@ -591,4 +591,66 @@ extern void __wait_lookup_done (void) attribute_hidden;
> > > # define USE_REQUEUE_PI(mut) 0
> > > #endif
> > >
> > > +/* Returns 0 if POL is a valid scheduling policy. */
> > > +static inline int
> > > +check_sched_policy_attr (int pol)
> > > +{
> > > + if (pol == SCHED_OTHER || pol == SCHED_FIFO || pol == SCHED_RR)
> > > + return 0;
> > > +
> > > + return EINVAL;
> > > +}
> > > +
> > > +/* Returns 0 if PR is within the accepted range of priority values for
> > > + the scheduling policy POL or EINVAL otherwise. */
> > > +static inline int
> > > +check_sched_priority_attr (int pr, int pol)
> > > +{
> > > + int min = __sched_get_priority_min (pol);
> > > + int max = __sched_get_priority_max (pol);
> > > +
> > > + if (min >= 0 && max >= 0 && pr >= min && pr <= max)
> > > + return 0;
> > > +
> > > + return EINVAL;
> > > +}
> > > +
> > > +/* Returns 0 if ST is a valid stack size for a thread stack and EINVAL
> > > + otherwise. */
> > > +static inline int
> > > +check_stacksize_attr (size_t st)
> > > +{
> > > + if (st >= PTHREAD_STACK_MIN)
> > > + return 0;
> > > +
> > > + return EINVAL;
> > > +}
> > > +
> > > +/* Defined in pthread_setaffinity.c. */
> > > +extern size_t __kernel_cpumask_size attribute_hidden;
> > > +extern int __determine_cpumask_size (pid_t tid);
> > > +
> > > +/* Returns 0 if CS and SZ are valid values for the cpuset and cpuset size
> > > + respectively. Otherwise it returns an error number. */
> > > +static inline int
> > > +check_cpuset_attr (const cpu_set_t *cs, const size_t sz)
> > > +{
> > > + if (__kernel_cpumask_size == 0)
> > > + {
> > > + int res = __determine_cpumask_size (THREAD_SELF->tid);
> > > + if (res)
> > > + return res;
> > > + }
> > > +
> > > + /* Check whether the new bitmask has any bit set beyond the
> > > + last one the kernel accepts. */
> > > + for (size_t cnt = __kernel_cpumask_size; cnt < sz; ++cnt)
> > > + if (((char *) cs)[cnt] != '\0')
> > > + /* Found a nonzero byte. This means the user request cannot be
> > > + fulfilled. */
> > > + return EINVAL;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > #endif /* pthreadP.h */
> > > diff --git a/nptl/pthread_attr_setschedparam.c b/nptl/pthread_attr_setschedparam.c
> > > index ec1a8bd..a167f15 100644
> > > --- a/nptl/pthread_attr_setschedparam.c
> > > +++ b/nptl/pthread_attr_setschedparam.c
> > > @@ -30,11 +30,10 @@ __pthread_attr_setschedparam (attr, param)
> > > assert (sizeof (*attr) >= sizeof (struct pthread_attr));
> > > struct pthread_attr *iattr = (struct pthread_attr *) attr;
> > >
> > > - int min = sched_get_priority_min (iattr->schedpolicy);
> > > - int max = sched_get_priority_max (iattr->schedpolicy);
> > > - if (min == -1 || max == -1
> > > - || param->sched_priority > max || param->sched_priority < min)
> > > - return EINVAL;
> > > + int ret = check_sched_priority_attr (param->sched_priority,
> > > + iattr->schedpolicy);
> > > + if (ret)
> > > + return ret;
> > >
> > > /* Copy the new values. */
> > > memcpy (&iattr->schedparam, param, sizeof (struct sched_param));
> > > diff --git a/nptl/pthread_attr_setschedpolicy.c b/nptl/pthread_attr_setschedpolicy.c
> > > index 2fa6921..4fe0b8e 100644
> > > --- a/nptl/pthread_attr_setschedpolicy.c
> > > +++ b/nptl/pthread_attr_setschedpolicy.c
> > > @@ -32,8 +32,9 @@ __pthread_attr_setschedpolicy (attr, policy)
> > > iattr = (struct pthread_attr *) attr;
> > >
> > > /* Catch invalid values. */
> > > - if (policy != SCHED_OTHER && policy != SCHED_FIFO && policy != SCHED_RR)
> > > - return EINVAL;
> > > + int ret = check_sched_policy_attr (policy);
> > > + if (ret)
> > > + return ret;
> > >
> > > /* Store the new values. */
> > > iattr->schedpolicy = policy;
> > > diff --git a/nptl/pthread_attr_setstack.c b/nptl/pthread_attr_setstack.c
> > > index 783cffe..4bd314e 100644
> > > --- a/nptl/pthread_attr_setstack.c
> > > +++ b/nptl/pthread_attr_setstack.c
> > > @@ -39,8 +39,9 @@ __pthread_attr_setstack (attr, stackaddr, stacksize)
> > > iattr = (struct pthread_attr *) attr;
> > >
> > > /* Catch invalid sizes. */
> > > - if (stacksize < PTHREAD_STACK_MIN)
> > > - return EINVAL;
> > > + int ret = check_stacksize_attr (stacksize);
> > > + if (ret)
> > > + return ret;
> > >
> > > #ifdef EXTRA_PARAM_CHECKS
> > > EXTRA_PARAM_CHECKS;
> > > diff --git a/nptl/pthread_attr_setstacksize.c b/nptl/pthread_attr_setstacksize.c
> > > index b7f2919..585bf08 100644
> > > --- a/nptl/pthread_attr_setstacksize.c
> > > +++ b/nptl/pthread_attr_setstacksize.c
> > > @@ -37,8 +37,9 @@ __pthread_attr_setstacksize (attr, stacksize)
> > > iattr = (struct pthread_attr *) attr;
> > >
> > > /* Catch invalid sizes. */
> > > - if (stacksize < PTHREAD_STACK_MIN)
> > > - return EINVAL;
> > > + int ret = check_stacksize_attr (stacksize);
> > > + if (ret)
> > > + return ret;
> > >
> > > iattr->stacksize = stacksize;
> > >
> > > diff --git a/nptl/sysdeps/unix/sysv/linux/pthread_attr_setaffinity.c b/nptl/sysdeps/unix/sysv/linux/pthread_attr_setaffinity.c
> > > index 8b7a499..b4335c5 100644
> > > --- a/nptl/sysdeps/unix/sysv/linux/pthread_attr_setaffinity.c
> > > +++ b/nptl/sysdeps/unix/sysv/linux/pthread_attr_setaffinity.c
> > > @@ -25,9 +25,6 @@
> > > #include <shlib-compat.h>
> > >
> > >
> > > -/* Defined in pthread_setaffinity.c. */
> > > -extern size_t __kernel_cpumask_size attribute_hidden;
> > > -extern int __determine_cpumask_size (pid_t tid);
> > >
> > >
> > > int
> > > @@ -47,21 +44,10 @@ __pthread_attr_setaffinity_new (pthread_attr_t *attr, size_t cpusetsize,
> > > }
> > > else
> > > {
> > > - if (__kernel_cpumask_size == 0)
> > > - {
> > > - int res = __determine_cpumask_size (THREAD_SELF->tid);
> > > - if (res != 0)
> > > - /* Some serious problem. */
> > > - return res;
> > > - }
> > > + int ret = check_cpuset_attr (cpuset, cpusetsize);
> > >
> > > - /* Check whether the new bitmask has any bit set beyond the
> > > - last one the kernel accepts. */
> > > - for (size_t cnt = __kernel_cpumask_size; cnt < cpusetsize; ++cnt)
> > > - if (((char *) cpuset)[cnt] != '\0')
> > > - /* Found a nonzero byte. This means the user request cannot be
> > > - fulfilled. */
> > > - return EINVAL;
> > > + if (ret)
> > > + return ret;
> > >
> > > if (iattr->cpusetsize != cpusetsize)
> > > {