[hurd,commited] hurd: Fix poll and select POSIX compliancy details about errors

[Samuel Thibault <samuel dot thibault at ens-lyon dot org>]

This fixes the following:

- On error, poll must not return without polling, including EBADF, and instead
report POLLHUP/POLLERR/POLLNVAL
- Select must report EBADF if some set contains an invalid FD.

The idea is to move error management to after all select calls, in the
poll/select final treatment. The error is instead recorded in a new `error'
field, and a new SELECT_ERROR bit set.

Thanks Svante Signell for the initial version of the patch.

	* hurd/hurdselect.c (SELECT_ERROR): New macro.
	(_hurd_select):
	- Add `error' field to `d' structures array.
	- If a poll descriptor is bogus, set EBADF, but continue with a zero
	timeout.
	- Go through the whole fd_set, not only until _hurd_dtablesize. Return
	EBADF there is any bit set above _hurd_dtablesize.
	- Do not request io_select on bogus descriptors (SELECT_ERROR).
	- On io_select request error, record the error.
	- On io_select bogus reply, use EIO error code.
	- On io_select bogus or error reply, record the error.
	- Do not destroy reply port for bogus FDs.
	- On error, make poll set POLLHUP in the EPIPE case, POLLNVAL in the
	EBADF case, or else POLLERR.
	- On error, make select simulated readiness.
---
 ChangeLog         |  15 +++++
 hurd/hurdselect.c | 149 +++++++++++++++++++++++++++++++++-------------
 2 files changed, 123 insertions(+), 41 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 37cbe28169..afd99a634e 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -4,6 +4,21 @@
 	(_hurd_canonicalize_directory_name_internal): Do not remove the heading
 	slash if we got an unknown root directory. (__getcwd): Do not fail with
 	EGRATUITOUS if we got an unknown root directory.
+	* hurd/hurdselect.c (SELECT_ERROR): New macro.
+	(_hurd_select):
+	- Add `error' field to `d' structures array.
+	- If a poll descriptor is bogus, set EBADF, but continue with a zero
+	timeout.
+	- Go through the whole fd_set, not only until _hurd_dtablesize. Return
+	EBADF there is any bit set above _hurd_dtablesize.
+	- Do not request io_select on bogus descriptors (SELECT_ERROR).
+	- On io_select request error, record the error.
+	- On io_select bogus reply, use EIO error code.
+	- On io_select bogus or error reply, record the error.
+	- Do not destroy reply port for bogus FDs.
+	- On error, make poll set POLLHUP in the EPIPE case, POLLNVAL in the
+	EBADF case, or else POLLERR.
+	- On error, make select simulated readiness.
 
 2019-08-30  Richard Braun  <rbraun@sceen.net>
 
diff --git a/hurd/hurdselect.c b/hurd/hurdselect.c
index 416e4a37bd..5a0de51ea5 100644
--- a/hurd/hurdselect.c
+++ b/hurd/hurdselect.c
@@ -34,6 +34,7 @@
 /* Used to record that a particular select rpc returned.  Must be distinct
    from SELECT_ALL (which better not have the high bit set).  */
 #define SELECT_RETURNED ((SELECT_ALL << 1) & ~SELECT_ALL)
+#define SELECT_ERROR (SELECT_RETURNED << 1)
 
 /* Check the first NFDS descriptors either in POLLFDS (if nonnnull) or in
    each of READFDS, WRITEFDS, EXCEPTFDS that is nonnull.  If TIMEOUT is not
@@ -61,6 +62,7 @@ _hurd_select (int nfds,
       mach_port_t io_port;
       int type;
       mach_port_t reply_port;
+      int error;
     } d[nfds];
   sigset_t oset;
 
@@ -118,6 +120,7 @@ _hurd_select (int nfds,
 
   if (pollfds)
     {
+      int error = 0;
       /* Collect interesting descriptors from the user's `pollfd' array.
 	 We do a first pass that reads the user's array before taking
 	 any locks.  The second pass then only touches our own stack,
@@ -151,28 +154,47 @@ _hurd_select (int nfds,
 	    if (fd < _hurd_dtablesize)
 	      {
 		d[i].cell = _hurd_dtable[fd];
-		d[i].io_port = _hurd_port_get (&d[i].cell->port, &d[i].ulink);
-		if (d[i].io_port != MACH_PORT_NULL)
-		  continue;
+		if (d[i].cell != NULL)
+		  {
+		    d[i].io_port = _hurd_port_get (&d[i].cell->port,
+						   &d[i].ulink);
+		    if (d[i].io_port != MACH_PORT_NULL)
+		      continue;
+		  }
 	      }
 
-	    /* If one descriptor is bogus, we fail completely.  */
-	    while (i-- > 0)
-	      if (d[i].type != 0)
-		_hurd_port_free (&d[i].cell->port,
-				 &d[i].ulink, d[i].io_port);
-	    break;
+	    /* Bogus descriptor, make it EBADF already.  */
+	    d[i].error = EBADF;
+	    d[i].type = SELECT_ERROR;
+	    error = 1;
 	  }
 
       __mutex_unlock (&_hurd_dtable_lock);
       HURD_CRITICAL_END;
 
-      if (i < nfds)
+      if (error)
 	{
-	  if (sigmask)
-	    __sigprocmask (SIG_SETMASK, &oset, NULL);
-	  errno = EBADF;
-	  return -1;
+	  /* Set timeout to 0.  */
+	  struct timeval now;
+	  err = __gettimeofday(&now, NULL);
+	  if (err)
+	    {
+	      /* Really bad luck.  */
+	      err = errno;
+	      HURD_CRITICAL_BEGIN;
+	      __mutex_lock (&_hurd_dtable_lock);
+	      while (i-- > 0)
+		if (d[i].type & ~SELECT_ERROR != 0)
+		  _hurd_port_free (&d[i].cell->port, &d[i].ulink,
+				   d[i].io_port);
+	      __mutex_unlock (&_hurd_dtable_lock);
+	      HURD_CRITICAL_END;
+	      errno = err;
+	      return -1;
+	    }
+	  ts.tv_sec = now.tv_sec;
+	  ts.tv_nsec = now.tv_usec * 1000;
+	  reply_msgid = IO_SELECT_TIMEOUT_REPLY_MSGID;
 	}
 
       lastfd = i - 1;
@@ -199,9 +221,6 @@ _hurd_select (int nfds,
       HURD_CRITICAL_BEGIN;
       __mutex_lock (&_hurd_dtable_lock);
 
-      if (nfds > _hurd_dtablesize)
-	nfds = _hurd_dtablesize;
-
       /* Collect the ports for interesting FDs.  */
       firstfd = lastfd = -1;
       for (i = 0; i < nfds; ++i)
@@ -216,9 +235,15 @@ _hurd_select (int nfds,
 	  d[i].type = type;
 	  if (type)
 	    {
-	      d[i].cell = _hurd_dtable[i];
-	      d[i].io_port = _hurd_port_get (&d[i].cell->port, &d[i].ulink);
-	      if (d[i].io_port == MACH_PORT_NULL)
+	      if (i < _hurd_dtablesize)
+		{
+		  d[i].cell = _hurd_dtable[i];
+		  if (d[i].cell != NULL)
+		    d[i].io_port = _hurd_port_get (&d[i].cell->port,
+						   &d[i].ulink);
+		}
+	      if (i >= _hurd_dtablesize || d[i].cell == NULL ||
+		  d[i].io_port == MACH_PORT_NULL)
 		{
 		  /* If one descriptor is bogus, we fail completely.  */
 		  while (i-- > 0)
@@ -243,6 +268,9 @@ _hurd_select (int nfds,
 	  errno = EBADF;
 	  return -1;
 	}
+
+      if (nfds > _hurd_dtablesize)
+	nfds = _hurd_dtablesize;
     }
 
 
@@ -260,7 +288,9 @@ _hurd_select (int nfds,
       portset = MACH_PORT_NULL;
 
       for (i = firstfd; i <= lastfd; ++i)
-	if (d[i].type)
+	if (!(d[i].type & ~SELECT_ERROR))
+	  d[i].reply_port = MACH_PORT_NULL;
+	else
 	  {
 	    int type = d[i].type;
 	    d[i].reply_port = __mach_reply_port ();
@@ -294,11 +324,10 @@ _hurd_select (int nfds,
 	      }
 	    else
 	      {
-		/* No error should happen.  Callers of select
-		   don't expect to see errors, so we simulate
-		   readiness of the erring object and the next call
-		   hopefully will get the error again.  */
-		d[i].type |= SELECT_RETURNED;
+		/* No error should happen, but record it for later
+		   processing.  */
+		d[i].error = err;
+		d[i].type |= SELECT_ERROR;
 		++got;
 	      }
 	    _hurd_port_free (&d[i].cell->port, &d[i].ulink, d[i].io_port);
@@ -404,9 +433,10 @@ _hurd_select (int nfds,
 #endif
 		  || msg.head.msgh_size != sizeof msg.success)
 		{
-		  /* Error or bogus reply.  Simulate readiness.  */
+		  /* Error or bogus reply.  */
+		  if (!msg.error.err)
+		    msg.error.err = EIO;
 		  __mach_msg_destroy (&msg.head);
-		  msg.success.result = SELECT_ALL;
 		}
 
 	      /* Look up the respondent's reply port and record its
@@ -418,9 +448,18 @@ _hurd_select (int nfds,
 		    if (d[i].type
 			&& d[i].reply_port == msg.head.msgh_local_port)
 		      {
-			d[i].type &= msg.success.result;
-			if (d[i].type)
-			  ++ready;
+			if (msg.error.err)
+			  {
+			    d[i].error = msg.error.err;
+			    d[i].type = SELECT_ERROR;
+			    ++ready;
+			  }
+			else
+			  {
+			    d[i].type &= msg.success.result;
+			    if (d[i].type)
+			      ++ready;
+			  }
 
 			d[i].type |= SELECT_RETURNED;
 			++got;
@@ -454,7 +493,7 @@ _hurd_select (int nfds,
 
   if (firstfd != -1)
     for (i = firstfd; i <= lastfd; ++i)
-      if (d[i].type)
+      if (d[i].reply_port != MACH_PORT_NULL)
 	__mach_port_destroy (__mach_task_self (), d[i].reply_port);
   if (firstfd == -1 || (firstfd != lastfd && portset != MACH_PORT_NULL))
     /* Destroy PORTSET, but only if it's not actually the reply port for a
@@ -476,15 +515,29 @@ _hurd_select (int nfds,
 	int type = d[i].type;
 	int_fast16_t revents = 0;
 
-	if (type & SELECT_RETURNED)
-	  {
-	    if (type & SELECT_READ)
-	      revents |= POLLIN;
-	    if (type & SELECT_WRITE)
-	      revents |= POLLOUT;
-	    if (type & SELECT_URG)
-	      revents |= POLLPRI;
-	  }
+	if (type & SELECT_ERROR)
+	  switch (d[i].error)
+	    {
+	      case EPIPE:
+		revents = POLLHUP;
+		break;
+	      case EBADF:
+		revents = POLLNVAL;
+		break;
+	      default:
+		revents = POLLERR;
+		break;
+	    }
+	else
+	  if (type & SELECT_RETURNED)
+	    {
+	      if (type & SELECT_READ)
+		revents |= POLLIN;
+	      if (type & SELECT_WRITE)
+		revents |= POLLOUT;
+	      if (type & SELECT_URG)
+		revents |= POLLPRI;
+	    }
 
 	pollfds[i].revents = revents;
       }
@@ -504,6 +557,20 @@ _hurd_select (int nfds,
 	    if ((type & SELECT_RETURNED) == 0)
 	      type = 0;
 
+	    /* Callers of select don't expect to see errors, so we simulate
+	       readiness of the erring object and the next call hopefully
+	       will get the error again.  */
+	    if (type & SELECT_ERROR)
+	      {
+		type = 0;
+		if (readfds != NULL && FD_ISSET (i, readfds))
+		  type |= SELECT_READ;
+		if (writefds != NULL && FD_ISSET (i, writefds))
+		  type |= SELECT_WRITE;
+		if (exceptfds != NULL && FD_ISSET (i, exceptfds))
+		  type |= SELECT_URG;
+	      }
+
 	    if (type & SELECT_READ)
 	      ready++;
 	    else if (readfds)
-- 
2.23.0.rc1