[PING,v2] Remove check for NULL buffer passed to `ptsname_r'

  ----- Forwarded message from Arjun Shankar <arjun.is@lostca.se> -----

Date: Mon, 29 May 2017 15:46:48 +0000
From: Arjun Shankar <arjun.is@lostca.se>
To: libc-alpha@sourceware.org
Cc: Joseph Myers <joseph@codesourcery.com>, Florian Weimer <fweimer@redhat.com>
Subject: [PATCH v2] Remove check for NULL buffer passed to `ptsname_r'

`ptsname_r' is declared in stdlib.h to only accept a `nonnull'
second argument and therefore GCC may choose to make optimizations
based on the assumption that this argument is NULL. This means
that potentially, GCC can optimize away the NULL check at some
point in the future. Since this is a programming interface, we
might as well remove the NULL check ourselves.

This also warrants a change to the `ptsname_r' manual page that
must be submitted to the corresponding mailing list.

In addition, remove the NULL buffer test in login/tst-ptsname.c.

ChangeLog:

2017-05-29  Arjun Shankar  <arjun.is@lostca.se>

	* sysdeps/unix/sysv/linux/ptsname.c (__ptsname_internal):
	Remove check for NULL 'buf'.
	* login/tst-ptsname.c (do_test): Remove test with NULL 'buf'.
---
Discussion on PATCH v1:
https://sourceware.org/ml/libc-alpha/2017-05/msg00726.html

 login/tst-ptsname.c               | 1 -
 sysdeps/unix/sysv/linux/ptsname.c | 6 ------
 2 files changed, 7 deletions(-)
  

> `ptsname_r' is declared in stdlib.h to only accept a `nonnull'
> second argument and therefore GCC may choose to make optimizations
> based on the assumption that this argument is NULL. This means
> that potentially, GCC can optimize away the NULL check at some
> point in the future. Since this is a programming interface, we
> might as well remove the NULL check ourselves.
>
> This also warrants a change to the `ptsname_r' manual page that
> must be submitted to the corresponding mailing list.

Is this function documented in our manual (manual/*.texi)? If so,
please update that.

OK with that change.  I don't think we need a copyright assignment for
a change that deletes seven lines of code and adds none.

zw
  

On 06/07/2017 03:39 PM, Zack Weinberg wrote:
>> `ptsname_r' is declared in stdlib.h to only accept a `nonnull'
>> second argument and therefore GCC may choose to make optimizations
>> based on the assumption that this argument is NULL. This means
>> that potentially, GCC can optimize away the NULL check at some
>> point in the future. Since this is a programming interface, we
>> might as well remove the NULL check ourselves.
>>
>> This also warrants a change to the `ptsname_r' manual page that
>> must be submitted to the corresponding mailing list.
> 
> Is this function documented in our manual (manual/*.texi)? If so,
> please update that.

Our manual doesn't say what happens with a NULL buffer, so no update is
needed.

> OK with that change.  I don't think we need a copyright assignment for
> a change that deletes seven lines of code and adds none.

Arjun's work is covered.

Thanks,
Florian
  

On Wed, Jun 07, 2017 at 09:39:40AM -0400, Zack Weinberg wrote:
> > `ptsname_r' is declared in stdlib.h to only accept a `nonnull'
> > second argument and therefore GCC may choose to make optimizations
> > based on the assumption that this argument is NULL. This means
> > that potentially, GCC can optimize away the NULL check at some
> > point in the future. Since this is a programming interface, we
> > might as well remove the NULL check ourselves.
> >
> > This also warrants a change to the `ptsname_r' manual page that
> > must be submitted to the corresponding mailing list.
> 
> Is this function documented in our manual (manual/*.texi)? If so,
> please update that.

The function is documented, but there is no reference to EINVAL and no
hint that a NULL buf is handled in any way. Here is the relevant text
(source is in manual/terminal.texi):

>  -- Function: int ptsname_r (int FILEDES, char *BUF, size_t LEN)
>      Preliminary: | MT-Safe | AS-Unsafe heap/bsd | AC-Unsafe mem fd |
>      *Note POSIX Safety Concepts::.
> 
>      The ptsname_r function is similar to the ptsname function
>      except that it places its result into the user-specified buffer
>      starting at BUF with length LEN.
> 
>      This function is a GNU extension.

Given this, I propose that we leave the this section unchanged. Instead, I
will propose a change to the man-pages project where `man ptsname_r'
documents this (now unhandled) error condition:

> EINVAL (ptsname_r() only) buf is NULL.

...once I commit this change.

Does the above sound like a reasonable course of action?

> OK with that change.  I don't think we need a copyright assignment for
> a change that deletes seven lines of code and adds none.

Even otherwise, I have an assignment on file, and I recently got commit
access in git :)

Cheers,
Arjun
  

On Wed, Jun 7, 2017 at 10:09 AM, Arjun Shankar <arjun.is@lostca.se> wrote:
> On Wed, Jun 07, 2017 at 09:39:40AM -0400, Zack Weinberg wrote:
>>
>> Is this function documented in our manual (manual/*.texi)? If so,
>> please update that.
>
> The function is documented, but there is no reference to EINVAL and no
> hint that a NULL buf is handled in any way. Here is the relevant text
> (source is in manual/terminal.texi):
>
>>  -- Function: int ptsname_r (int FILEDES, char *BUF, size_t LEN)
>>      Preliminary: | MT-Safe | AS-Unsafe heap/bsd | AC-Unsafe mem fd |
>>      *Note POSIX Safety Concepts::.
>>
>>      The ptsname_r function is similar to the ptsname function
>>      except that it places its result into the user-specified buffer
>>      starting at BUF with length LEN.
>>
>>      This function is a GNU extension.
>
> Given this, I propose that we leave the this section unchanged.

I agree.  We don't normally bother saying that passing a null pointer
to a function may cause a crash - in fact, it's the other way around,
we specifically document when null *is* allowed.

>> OK with that change.  I don't think we need a copyright assignment for
>> a change that deletes seven lines of code and adds none.
>
> Even otherwise, I have an assignment on file, and I recently got commit
> access in git :)

Oh, sorry about that.  I didn't recognize your name.

zw