Use an alias to `ptsname_r' in login/tst-ptsname.c
diff mbox

Message ID 20170524155000.GA45792@aloka.lostca.se
State New
Headers show

Commit Message

Arjun Shankar May 24, 2017, 3:50 p.m. UTC
When compiled with `-O3', inlining causes GCC to realize that one of the
tests calls `ptsname_r' with a NULL buffer and error out when compiling
with -Wnonnull.

Declare `ptsname_r_alias' and alias it to `ptsname_r' to avoid this.

ChangeLog:

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

	* login/tst-ptsname.c (ptsname_r_alias): New declaration.
	(do_single_test): Use ptsname_r_alias instead of ptsname_r.
---
 login/tst-ptsname.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Joseph Myers May 24, 2017, 4:18 p.m. UTC | #1
On Wed, 24 May 2017, Arjun Shankar wrote:

> When compiled with `-O3', inlining causes GCC to realize that one of the
> tests calls `ptsname_r' with a NULL buffer and error out when compiling
> with -Wnonnull.
> 
> Declare `ptsname_r_alias' and alias it to `ptsname_r' to avoid this.

Normally in such cases we'd use the DIAG_* macros from libc-diag.h, with 
appropriate comments, to disable the specific warning for the code in 
question.
Florian Weimer May 24, 2017, 5:42 p.m. UTC | #2
On 05/24/2017 06:18 PM, Joseph Myers wrote:
> On Wed, 24 May 2017, Arjun Shankar wrote:
> 
>> When compiled with `-O3', inlining causes GCC to realize that one of the
>> tests calls `ptsname_r' with a NULL buffer and error out when compiling
>> with -Wnonnull.
>>
>> Declare `ptsname_r_alias' and alias it to `ptsname_r' to avoid this.
> 
> Normally in such cases we'd use the DIAG_* macros from libc-diag.h, with 
> appropriate comments, to disable the specific warning for the code in 
> question.

I think passing NULL to a nonnull parameter is undefined, warning or
not.  The GCC documentation is pretty clear about that.  (I have some
doubts about the validity of the NULL comparison inside the
implementation of ptsname_r, too.)

Alternatively, we could withdraw the test as invalid, and update the
manual page.

Thanks,
Florian
Arjun Shankar May 25, 2017, 4:05 p.m. UTC | #3
On Wed, May 24, 2017 at 07:42:37PM +0200, Florian Weimer wrote:
> I think passing NULL to a nonnull parameter is undefined, warning or
> not.  The GCC documentation is pretty clear about that.  (I have some
> doubts about the validity of the NULL comparison inside the
> implementation of ptsname_r, too.)
> 
> Alternatively, we could withdraw the test as invalid, and update the
> manual page.

Removing the sub-test that tests for EINVAL on NULL sounds like a good
idea to me as well. About the man-page I just looked at
`pthread_create' for comparison:

* the `pthread_t *' argument is declared `nonnull' in pthread.h
* there is no code in `pthread_create' to check if the argument is NULL
* nothing in the man page to indicate what happens when NULL is passed
* compiling `pthread_create (NULL, NULL, NULL, NULL)' without
`-Wall -Werror' produces a binary that SIGSEGVs
* compiling with `-Wall -Werror' errors out the compilation

Given this, maybe in the case of `ptsname_r' it is enough to just drop
the specific test that tests with NULL, and leave the man-page
unchanged? While we are at it, we could also drop the test for NULL in
`ptsname_r'. How does that sound for a PATCH v2?
Florian Weimer May 25, 2017, 4:22 p.m. UTC | #4
On 05/25/2017 06:05 PM, Arjun Shankar wrote:

> Given this, maybe in the case of `ptsname_r' it is enough to just drop
> the specific test that tests with NULL, and leave the man-page
> unchanged?

That's fine as well.  I don't know if others want to preserve the EINVAL
handling.

> While we are at it, we could also drop the test for NULL in
> `ptsname_r'. How does that sound for a PATCH v2?

That would require updating the manpage, wouldn't it?

Thanks,
Florian
Arjun Shankar May 25, 2017, 9:37 p.m. UTC | #5
> > Given this, maybe in the case of `ptsname_r' it is enough to just drop
> > the specific test that tests with NULL, and leave the man-page
> > unchanged?
> 
> That's fine as well.  I don't know if others want to preserve the EINVAL
> handling.
> 
> > While we are at it, we could also drop the test for NULL in
> > `ptsname_r'. How does that sound for a PATCH v2?
> 
> That would require updating the manpage, wouldn't it?

My argument behind both of these suggestions was that `pthread_create'
doesn't care about both of these things (i.e. it doesn't check that the
`pthread_t' pointer is not NULL; it just writes to it. And it doesn't
explicitly mention in the man-page that the pointer is required to be
valid and of sufficient size) - and therefore `ptsname_r' doesn't
require these things either.

Am I reasoning about this correctly?
Florian Weimer May 25, 2017, 9:40 p.m. UTC | #6
On Thu, May 25, 2017 at 11:37 PM, Arjun Shankar <arjun.is@lostca.se> wrote:
> My argument behind both of these suggestions was that `pthread_create'
> doesn't care about both of these things (i.e. it doesn't check that the
> `pthread_t' pointer is not NULL; it just writes to it. And it doesn't
> explicitly mention in the man-page that the pointer is required to be
> valid and of sufficient size) - and therefore `ptsname_r' doesn't
> require these things either.
>
> Am I reasoning about this correctly?

What you are saying is not wrong, but the manual page in its current
state documents the EINVAL return value.  That would have to change.

Thanks,
Florian

Patch
diff mbox

diff --git a/login/tst-ptsname.c b/login/tst-ptsname.c
index be8744d..dbc8d19 100644
--- a/login/tst-ptsname.c
+++ b/login/tst-ptsname.c
@@ -27,11 +27,16 @@ 
 #define DEV_TTY		"/dev/tty"
 #define PTSNAME_EINVAL	"./ptsname-einval"
 
+/* When compiled with `-O3', inlining causes GCC to realize that one of the
+   tests calls `ptsname_r' with a NULL buffer and error out when compiling
+   with -Wnonnull. To avoid this, we use an alias instead.  */
+int ptsname_r_alias (int, char *, size_t) __asm__ ("ptsname_r");
+
 static int
 do_single_test (int fd, char *buf, size_t buflen, int expected_err)
 {
 
-  int ret = ptsname_r (fd, buf, buflen);
+  int ret = ptsname_r_alias (fd, buf, buflen);
   int err = errno;
 
   if (expected_err == 0)