pthread_create: fix segfault when attr is NULL

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

pthread_create: fix segfault when attr is NULL

Paul Irofti-4
Hi,

The current implementation does not check for NULL attr values and
segfaults when creating new threads if that's the case:

Program received signal SIGSEGV, Segmentation fault.
0x00000a77e9550ade in pthread_create (threadp=0x7f7ffffe5488, attr=0x7f7ffffe5480,
    start_routine=0xa7539a00600 <a_thread_func>, arg=<optimized out>) at /usr/src/lib/librthread/rthread.c:370
370             thread->attr = attr != NULL ? *(*attr) : _rthread_attr_default;
(gdb) p attr
$1 = (const pthread_attr_t *) 0x7f7ffffe5480
(gdb) p *attr
$2 = (const pthread_attr_t) 0x0


The diff bellow fixes the issue by checking *attr and returning EINVAL
if it is NULL. OK?

(Surprisingly the manual already documents this.)

Paul


Index: rthread.c
===================================================================
RCS file: /cvs/src/lib/librthread/rthread.c,v
retrieving revision 1.99
diff -u -p -u -p -r1.99 rthread.c
--- rthread.c 4 Nov 2017 22:53:57 -0000 1.99
+++ rthread.c 6 Jul 2018 12:04:07 -0000
@@ -367,6 +367,9 @@ pthread_create(pthread_t *threadp, const
  thread->arg = arg;
  tib->tib_tid = -1;
 
+ if (attr != NULL && *attr == NULL)
+ return (EINVAL);
+
  thread->attr = attr != NULL ? *(*attr) : _rthread_attr_default;
  if (thread->attr.sched_inherit == PTHREAD_INHERIT_SCHED) {
  pthread_t self = pthread_self();

Reply | Threaded
Open this post in threaded view
|

Re: pthread_create: fix segfault when attr is NULL

Mark Kettenis
> Date: Fri, 6 Jul 2018 15:05:56 +0300
> From: Paul Irofti <[hidden email]>
>
> Hi,
>
> The current implementation does not check for NULL attr values and
> segfaults when creating new threads if that's the case:
>
> Program received signal SIGSEGV, Segmentation fault.
> 0x00000a77e9550ade in pthread_create (threadp=0x7f7ffffe5488, attr=0x7f7ffffe5480,
>     start_routine=0xa7539a00600 <a_thread_func>, arg=<optimized out>) at /usr/src/lib/librthread/rthread.c:370
> 370             thread->attr = attr != NULL ? *(*attr) : _rthread_attr_default;
> (gdb) p attr
> $1 = (const pthread_attr_t *) 0x7f7ffffe5480
> (gdb) p *attr
> $2 = (const pthread_attr_t) 0x0
>
>
> The diff bellow fixes the issue by checking *attr and returning EINVAL
> if it is NULL. OK?

POSIX currently says:

  The behavior is undefined if the value specified by the attr
  argument to pthread_create() does not refer to an initialized thread
  attributes object.

So your code is wrong and a segfault is perfectly fine behaviour.  I
think our policy still is to crash as quickly as possible in the case
of undefined behaviour.

> (Surprisingly the manual already documents this.)

You mean:

  [EINVAL]           The value specified by attr is invalid.

?  That isn't actually in the current version of POSIX, and I'm not
sure this covers a completely uninitialized attributes object.

So maybe we should change the man page instead?


> Index: rthread.c
> ===================================================================
> RCS file: /cvs/src/lib/librthread/rthread.c,v
> retrieving revision 1.99
> diff -u -p -u -p -r1.99 rthread.c
> --- rthread.c 4 Nov 2017 22:53:57 -0000 1.99
> +++ rthread.c 6 Jul 2018 12:04:07 -0000
> @@ -367,6 +367,9 @@ pthread_create(pthread_t *threadp, const
>   thread->arg = arg;
>   tib->tib_tid = -1;
>  
> + if (attr != NULL && *attr == NULL)
> + return (EINVAL);
> +
>   thread->attr = attr != NULL ? *(*attr) : _rthread_attr_default;
>   if (thread->attr.sched_inherit == PTHREAD_INHERIT_SCHED) {
>   pthread_t self = pthread_self();
>
>

Reply | Threaded
Open this post in threaded view
|

Re: pthread_create: fix segfault when attr is NULL

Paul Irofti-4
> POSIX currently says:
>
>   The behavior is undefined if the value specified by the attr
>   argument to pthread_create() does not refer to an initialized thread
>   attributes object.

I don't see that bit:

http://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_create.html

On the contrary, I see what our manual states:

  [EINVAL]
          The attributes specified by attr are invalid.

Reply | Threaded
Open this post in threaded view
|

Re: pthread_create: fix segfault when attr is NULL

Mark Kettenis
> Date: Fri, 6 Jul 2018 15:36:07 +0300
> From: Paul Irofti <[hidden email]>
>
> > POSIX currently says:
> >
> >   The behavior is undefined if the value specified by the attr
> >   argument to pthread_create() does not refer to an initialized thread
> >   attributes object.
>
> I don't see that bit:
>
> http://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_create.html
>
> On the contrary, I see what our manual states:
>
>   [EINVAL]
>  The attributes specified by attr are invalid.

http://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_create.html

Reply | Threaded
Open this post in threaded view
|

Re: pthread_create: fix segfault when attr is NULL

Scott Cheloha
On Fri, Jul 06, 2018 at 03:07:12PM +0200, Mark Kettenis wrote:

> > Date: Fri, 6 Jul 2018 15:36:07 +0300
> > From: Paul Irofti <[hidden email]>
> >
> > > POSIX currently says:
> > >
> > >   The behavior is undefined if the value specified by the attr
> > >   argument to pthread_create() does not refer to an initialized thread
> > >   attributes object.
> >
> > I don't see that bit:
> >
> > http://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_create.html
> >
> > On the contrary, I see what our manual states:
> >
> >   [EINVAL]
> >  The attributes specified by attr are invalid.
>
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_create.html

If we're gonna adhere to the latest behavior we ought to update the cited
standard from ISO/IEC 9945-1:1996 ("POSIX.1") to... something more recent.

ISO/IEC 9945-1:2009 contains the behavior you're describing, but most
of our manpages cite revisions of IEEE 1003.1.  Most of *those* cite
2008, but judging from your link the 2017 revision is the new hotness.

The distinction between ISO/IEC 9945-1 and IEEE 1003.1 is pretty
fuzzy to me.

So, which standard/revision is the preferred citation target, if any?

And should we update all of the pthread manpages in one sweep?

cc jmc

PS and by "we" I totally mean "I".  How hard could it be? :)

Reply | Threaded
Open this post in threaded view
|

Re: pthread_create: fix segfault when attr is NULL

Ingo Schwarze
Hi,

Scott Cheloha wrote on Fri, Jul 06, 2018 at 07:31:36PM -0500:
> On Fri, Jul 06, 2018 at 03:07:12PM +0200, Mark Kettenis wrote:
>> Paul Irofti wrote on Fri, 6 Jul 2018 15:36:07 +0300:
>>> somebody wrote:

>>>> POSIX currently says:
>>>>
>>>>   The behavior is undefined if the value specified by the attr
>>>>   argument to pthread_create() does not refer to an initialized thread
>>>>   attributes object.

>>> I don't see that bit:
>>>
>>> http://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_create.html

That is -p1003.1-2004, an old version of the standard.

>>> On the contrary, I see what our manual states:
>>>
>>>   [EINVAL]
>>>  The attributes specified by attr are invalid.

>> http://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_create.html

That is -p1003.1-2008, the current version of the standard.

> If we're gonna adhere to the latest behavior we ought to update the cited
> standard from ISO/IEC 9945-1:1996 ("POSIX.1") to... something more recent.
>
> ISO/IEC 9945-1:2009 contains the behavior you're describing, but most
> of our manpages cite revisions of IEEE 1003.1.  Most of *those* cite
> 2008, but judging from your link the 2017 revision is the new hotness.
>
> The distinction between ISO/IEC 9945-1 and IEEE 1003.1 is pretty
> fuzzy to me.
>
> So, which standard/revision is the preferred citation target, if any?

For library functions, we tend to cite the oldest revision of POSIX
that our implementation conforms to, that is, the first that matches
from the following list:

  -ansiC         \" 1989
  -isoC-99       \" 1990
  -isoC-2011     \" 2011

  -p1003.1       \" 1988
  -p1003.1-90    \" 1990
  -p1003.1-96    \" 1996

  -xpg3          \" 1989
  -xpg4          \" 1992
  -xpg4.2        \" 1994
  -susv2         \" 1997

  -p1003.1-2001  \" 2001
  -p1003.1-2004  \" 2004
  -p1003.1-2008  \" 2008

This approximate rule probably isn't followed completely consistently;
for example, instead of -xpg* and -susv2, some pages might directly
reference the later -p1003.1-2001 or -p1003.1-2004 that incorporated
them.  Some pages might also be more specific and reference e.g.
-p1003.1c-95 instead of -p1003.1-96.  Neither of that would seem
wrong to me.

Note that we will not introduce a new -p1003.1-20xx for the 2016
edition, which is merely incorporated technical corrigenda, not a
new revision of the standard.  It has proven very rare in practice
that the behaviour required by the 2016 edition differs from the
behaviour required by the 2008 edition.  In the odd case where it
does, that is such a bad trap that it ought to be decribed explictly
rather than merely with an .St argument that is easy to overlook.

> And should we update all of the pthread manpages in one sweep?

Any required corrections are welcome.

> cc jmc
> PS and by "we" I totally mean "I".  How hard could it be? :)

The reason such sweeps are rarely done for sections 2/3 is that jmc@
isn't comfortable doing them on his own in those sections.  Where
needed, such sweeps are certainly welcome.

He did the sweep in sections 1/8.  There, the policy is somewhat
different: There, we tend to reference the *newest* standard we
conform to from the above list.

Yours,
  Ingo

Reply | Threaded
Open this post in threaded view
|

Re: pthread_create: fix segfault when attr is NULL

Jason McIntyre-2
On Sat, Jul 07, 2018 at 07:42:44AM +0200, Ingo Schwarze wrote:

> Hi,
>
> Scott Cheloha wrote on Fri, Jul 06, 2018 at 07:31:36PM -0500:
> > On Fri, Jul 06, 2018 at 03:07:12PM +0200, Mark Kettenis wrote:
> >> Paul Irofti wrote on Fri, 6 Jul 2018 15:36:07 +0300:
> >>> somebody wrote:
>
> >>>> POSIX currently says:
> >>>>
> >>>>   The behavior is undefined if the value specified by the attr
> >>>>   argument to pthread_create() does not refer to an initialized thread
> >>>>   attributes object.
>
> >>> I don't see that bit:
> >>>
> >>> http://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_create.html
>
> That is -p1003.1-2004, an old version of the standard.
>
> >>> On the contrary, I see what our manual states:
> >>>
> >>>   [EINVAL]
> >>>  The attributes specified by attr are invalid.
>
> >> http://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_create.html
>
> That is -p1003.1-2008, the current version of the standard.
>
> > If we're gonna adhere to the latest behavior we ought to update the cited
> > standard from ISO/IEC 9945-1:1996 ("POSIX.1") to... something more recent.
> >
> > ISO/IEC 9945-1:2009 contains the behavior you're describing, but most
> > of our manpages cite revisions of IEEE 1003.1.  Most of *those* cite
> > 2008, but judging from your link the 2017 revision is the new hotness.
> >
> > The distinction between ISO/IEC 9945-1 and IEEE 1003.1 is pretty
> > fuzzy to me.
> >
> > So, which standard/revision is the preferred citation target, if any?
>
> For library functions, we tend to cite the oldest revision of POSIX
> that our implementation conforms to, that is, the first that matches
> from the following list:
>
>   -ansiC         \" 1989
>   -isoC-99       \" 1990
>   -isoC-2011     \" 2011
>
>   -p1003.1       \" 1988
>   -p1003.1-90    \" 1990
>   -p1003.1-96    \" 1996
>
>   -xpg3          \" 1989
>   -xpg4          \" 1992
>   -xpg4.2        \" 1994
>   -susv2         \" 1997
>
>   -p1003.1-2001  \" 2001
>   -p1003.1-2004  \" 2004
>   -p1003.1-2008  \" 2008
>
> This approximate rule probably isn't followed completely consistently;
> for example, instead of -xpg* and -susv2, some pages might directly
> reference the later -p1003.1-2001 or -p1003.1-2004 that incorporated
> them.  Some pages might also be more specific and reference e.g.
> -p1003.1c-95 instead of -p1003.1-96.  Neither of that would seem
> wrong to me.
>
> Note that we will not introduce a new -p1003.1-20xx for the 2016
> edition, which is merely incorporated technical corrigenda, not a
> new revision of the standard.  It has proven very rare in practice
> that the behaviour required by the 2016 edition differs from the
> behaviour required by the 2008 edition.  In the odd case where it
> does, that is such a bad trap that it ought to be decribed explictly
> rather than merely with an .St argument that is easy to overlook.
>
> > And should we update all of the pthread manpages in one sweep?
>
> Any required corrections are welcome.
>

agreed.

> > cc jmc
> > PS and by "we" I totally mean "I".  How hard could it be? :)
>
> The reason such sweeps are rarely done for sections 2/3 is that jmc@
> isn't comfortable doing them on his own in those sections.  Where
> needed, such sweeps are certainly welcome.
>
> He did the sweep in sections 1/8.  There, the policy is somewhat
> different: There, we tend to reference the *newest* standard we
> conform to from the above list.
>

the 2/3 pages should really reference the most recent standards too.
it's just the work hasn;t been done.

jmc

> Yours,
>   Ingo

Reply | Threaded
Open this post in threaded view
|

Re: pthread_create: fix segfault when attr is NULL

Ingo Schwarze
Hi Jason,

Jason McIntyre wrote on Sat, Jul 07, 2018 at 07:17:29AM +0100:

> the 2/3 pages should really reference the most recent standards too.
> it's just the work hasn;t been done.

According to my understanding, the difference in policy is deliberate.
Some people may want to write C code according to specific language
standards, so knowing that a feature is available in -ansiC is
useful even if it is also available in -p1003.1-2008.  In contrast,
writing shell scripts according to historic POSIX versions wouldn't
really make sense.

Yours,
  Ingo

Reply | Threaded
Open this post in threaded view
|

Re: pthread_create: fix segfault when attr is NULL

Jason McIntyre-2
On Sat, Jul 07, 2018 at 08:25:07AM +0200, Ingo Schwarze wrote:

> Hi Jason,
>
> Jason McIntyre wrote on Sat, Jul 07, 2018 at 07:17:29AM +0100:
>
> > the 2/3 pages should really reference the most recent standards too.
> > it's just the work hasn;t been done.
>
> According to my understanding, the difference in policy is deliberate.
> Some people may want to write C code according to specific language
> standards, so knowing that a feature is available in -ansiC is
> useful even if it is also available in -p1003.1-2008.  In contrast,
> writing shell scripts according to historic POSIX versions wouldn't
> really make sense.
>
> Yours,
>   Ingo
>

oh, i didn;t know that.
jmc

Reply | Threaded
Open this post in threaded view
|

Re: pthread_create: fix segfault when attr is NULL

Anthony J. Bentley-4
In reply to this post by Ingo Schwarze
Ingo Schwarze writes:

> Hi Jason,
>
> Jason McIntyre wrote on Sat, Jul 07, 2018 at 07:17:29AM +0100:
>
> > the 2/3 pages should really reference the most recent standards too.
> > it's just the work hasn;t been done.
>
> According to my understanding, the difference in policy is deliberate.
> Some people may want to write C code according to specific language
> standards, so knowing that a feature is available in -ansiC is
> useful even if it is also available in -p1003.1-2008.  In contrast,
> writing shell scripts according to historic POSIX versions wouldn't
> really make sense.

My memory of the last time we had this discussion (documenting the
multibyte functions if I recall correctly) is the same as jmc's, that it
would be preferable to target a single standard but nobody has sat down
and done the work.