Re: [PATCH][BZ #13724] Do not segfault in pthread_setname_np (x, NULL)

[Rich Felker <dalias at aerifal dot cx>]

On Thu, Oct 03, 2013 at 03:49:38PM -0400, Carlos O'Donell wrote:
> On 10/03/2013 08:20 AM, OndÅej BÃlka wrote:
> > Hi, this is another bug that could be handled quickly. Problem is if 
> > pthread_setname_np (x, NULL) should return error code or segfault. 
> > (see https://sourceware.org/bugzilla/show_bug.cgi?id=13724 )
> > If first is desired then following patch does it. If we choose for
> > second then close that bugzilla entry.
> > 
> > 	* sysdeps/unix/sysv/linux/pthread_setname.c (pthread_setname_np):
> > 	Handle null.
> > 
> > diff --git a/nptl/sysdeps/unix/sysv/linux/pthread_setname.c b/nptl/sysdeps/unix/sysv/linux/pthread_setname.c
> > index d6455dd..45eefa0 100644
> > --- a/nptl/sysdeps/unix/sysv/linux/pthread_setname.c
> > +++ b/nptl/sysdeps/unix/sysv/linux/pthread_setname.c
> > @@ -34,6 +34,10 @@ pthread_setname_np (th, name)
> >  {
> >    const struct pthread *pd = (const struct pthread *) th;
> >  
> > +  /* Return EFAULT like pthread_getname_np(x, NULL, 16) does.  */
> > +  if (name == NULL)
> > +    return EFAULT;
> > +
> >    /* Unfortunately the kernel headers do not export the TASK_COMM_LEN
> >       macro.  So we have to define it here.  */
> >  #define TASK_COMM_LEN 16
> > 
> 
> Thanks for tackling the BZs. You're doing a great job here.
> 
> I agree with Jeff Law on this one, this is not a performance
> critical routine and checking arguments and returning EFAULT
> is good for QoI.

I disagree; I think it hurts QoI. The basic argument (copied from
http://stackoverflow.com/questions/4390007/4390210#4390210) is:

    If you're going to check for NULL pointer arguments where you have
    not entered into a contract to accept and interpret them, do it
    with an assert, not a conditional error return. This way the bugs
    in the caller will be immediately detected and can be fixed, and
    it makes it easy to disable the overhead in production builds. I
    question the value of the assert except as documentation however;
    a segfault from dereferencing the NULL pointer is just as
    effective for debugging.

    If you return an error code to a caller which has already proven
    itself buggy, the most likely result is that the caller will
    ignore the error, and bad things will happen much later down the
    line when the original cause of the error has become difficult or
    impossible to track down. Why is it reasonable to assume the
    caller will ignore the error you return? Because the caller
    already ignored the error return of malloc or fopen or some other
    library-specific allocation function which returned NULL to
    indicate an error!

In the case of pthread_setname_np, it's unlikely that anything
horrible will happen by ignoring the error, but it's better for the
implementation to catch the buggy usage (passing NULL to a function
that's not supposed to accept NULL) than to ignore it. And returning
an error code is essentially ignoring it, for the above reason (the
calling program has already demonstrated that it's ignoring errors or
NULL would have never reached pthread_setname_np).

Rich