Discussion:
poll: enable argument check
(too old to reply)
Bruno Haible
2017-04-22 12:59:52 UTC
Permalink
Raw Message
On Mac OS X, I'm seeing this warning:


poll.c:338:11: warning: comparison of unsigned expression < 0 is always false [-Wtautological-compare]
if (nfd < 0)
~~~ ^ ~

Indeed, nfd is of type nfds_t, which is an unsigned integer type per POSIX [1].

This fixes it:

2017-04-22 Bruno Haible <***@clisp.org>

poll: Enable argument check.
* lib/poll.c: Include intprops.h.
(poll): Check value of nfd correctly.
* modules/poll (Depends-on): Add intprops.

diff --git a/lib/poll.c b/lib/poll.c
index 88d9292..803ac0e 100644
--- a/lib/poll.c
+++ b/lib/poll.c
@@ -60,6 +60,7 @@
#include <time.h>

#include "assure.h"
+#include "intprops.h"

#ifndef INFTIM
# define INFTIM (-1)
@@ -335,7 +336,7 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
int maxfd, rc;
nfds_t i;

- if (nfd < 0)
+ if (nfd > TYPE_MAXIMUM (nfds_t) / 2)
{
errno = EINVAL;
return -1;
diff --git a/modules/poll b/modules/poll
index 57f0631..a7d117f 100644
--- a/modules/poll
+++ b/modules/poll
@@ -9,6 +9,7 @@ Depends-on:
poll-h
alloca [test $HAVE_POLL = 0 || test $REPLACE_POLL = 1]
assure [test $HAVE_POLL = 0 || test $REPLACE_POLL = 1]
+intprops [test $HAVE_POLL = 0 || test $REPLACE_POLL = 1]
select [test $HAVE_POLL = 0 || test $REPLACE_POLL = 1]
sockets [test $HAVE_POLL = 0 || test $REPLACE_POLL = 1]
sys_select [test $HAVE_POLL = 0 || test $REPLACE_POLL = 1]



[1] http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/poll.h.html
Paul Eggert
2017-04-22 21:31:45 UTC
Permalink
Raw Message
Post by Bruno Haible
- if (nfd < 0)
+ if (nfd > TYPE_MAXIMUM (nfds_t) / 2)
This doesn't look right. What does TYPE_MAXIMUM (nfds_t) / 2 have to do with
anything? Plus, five lines later there's a comment that starts "Don't check
directly for NFD too large" and this seems to directly contradict the code now.
Plus, there's another "nfd < 0" in the WINDOWS_NATIVE case.

How about the attached patch?
Bruno Haible
2017-04-23 01:26:39 UTC
Permalink
Raw Message
Hi Paul,
Post by Paul Eggert
Post by Bruno Haible
- if (nfd < 0)
+ if (nfd > TYPE_MAXIMUM (nfds_t) / 2)
This doesn't look right. What does TYPE_MAXIMUM (nfds_t) / 2 have to do with
anything?
TYPE_MAXIMUM (nfds_t) / 2 is good for a quick check against an out-of-bounds nfd.
POSIX [1] says that nfd is out-of-bounds if it is > getdtablesize().
Post by Paul Eggert
Plus, five lines later there's a comment that starts "Don't check
directly for NFD too large" and this seems to directly contradict the code now.
This comment means that the code is a bit sloppy for nfd > getdtablesize().
IMO, this is OK for nfd < TYPE_MAXIMUM (nfds_t) / 2 because such values rarely
occur by accident. Whereas it's easy to pass -1 by mistake, which gets converted
to (nfds_t)-1.
Post by Paul Eggert
Plus, there's another "nfd < 0" in the WINDOWS_NATIVE case.
Oops. Thanks. I've corrected it now.
Post by Paul Eggert
How about the attached patch?
I disagree. It no longer catches the case nfd = (nfds_t)-1 quickly.

Bruno

[1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/poll.html
Paul Eggert
2017-04-23 01:53:19 UTC
Permalink
Raw Message
Post by Bruno Haible
TYPE_MAXIMUM (nfds_t) / 2 is good for a quick check against an out-of-bounds nfd.
POSIX [1] says that nfd is out-of-bounds if it is > getdtablesize().
OK, but getdtablesize returns 'int' so the 'poll' code should compare to INT_MAX
rather than to an expression that would not be appropriate on (admittedly weird)
platforms where nfds_t differs in width from 'int'. Also, that comment is still
confusing. How about the attached?
Bruno Haible
2017-04-23 09:02:22 UTC
Permalink
Raw Message
Post by Paul Eggert
OK, but getdtablesize returns 'int' so the 'poll' code should compare to INT_MAX
... How about the attached?
Thanks. I pushed it in your name, with an update of the dependencies list.

Bruno

Loading...