[PATCH] poll: fix regression in Win32 emulation of poll function

[Daniel P. Berrange]

The previous commit:

commit 17f1e64f00011fb745019119e21b26e4aba65e4b
Author: Paul Eggert <***@cs.ucla.edu>
Date: Tue Feb 24 16:16:19 2015 -0800

poll: port to MSVC v18 on MS-Windows 8.1

Problem reported by Gisle Vanem in:
http://lists.gnu.org/archive/html/bug-gnulib/2015-02/msg00139.html
* lib/poll.c: Always include <sys/select.h> and <sys/socket.h>.
* modules/poll (Depends-on) [!HAVE_POLL || REPLACE_POLL]:
Add sys_socket.

attempted to fix a compilation bug, but in doing so it significantly
changed the semantics of the code. Instead of the poll() wrapper
calling the WINSOCK native select() & recv() functions, it would
now call the GNULIB wrapped versions. This is a mistake because
the former take SOCKET handles, while the latter take FDs. As a
result, while the poll() code compiled, it didn't actually work
when used.

Rather than reverting the above commit, which would reintroduce
the compile error reporting, we simply undefine the wrappers
so the code calls the WINSOCK native functions once again.

Signed-off-by: Daniel P. Berrange <***@redhat.com>
---
lib/poll.c | 10 ++++++++++
1 file changed, 10 insertions(+)

diff --git a/lib/poll.c b/lib/poll.c
index c4b2127..a26838c 100644
--- a/lib/poll.c
+++ b/lib/poll.c
@@ -54,6 +54,16 @@
#include <sys/select.h>
#include <sys/socket.h>

+#ifdef WINDOWS_NATIVE
+/* The Win32 emulation assumes we're using the Winsock native
+ * functions that accept SOCKETs, not the GNULIB wrapped versions
+ * that accept FDs. The sys/select.h & sys/socket.h header pull
+ * in the wrapped versions, so we must unset them.
+ */
+# undef recv
+# undef select
+#endif
+
#ifdef HAVE_SYS_IOCTL_H
# include <sys/ioctl.h>
#endif

[Daniel P. Berrange]

Post by Daniel P. Berrange
commit 17f1e64f00011fb745019119e21b26e4aba65e4b
Date: Tue Feb 24 16:16:19 2015 -0800
poll: port to MSVC v18 on MS-Windows 8.1
http://lists.gnu.org/archive/html/bug-gnulib/2015-02/msg00139.html
* lib/poll.c: Always include <sys/select.h> and <sys/socket.h>.
Add sys_socket.
attempted to fix a compilation bug, but in doing so it significantly
changed the semantics of the code. Instead of the poll() wrapper
calling the WINSOCK native select() & recv() functions, it would
now call the GNULIB wrapped versions. This is a mistake because
the former take SOCKET handles, while the latter take FDs. As a
result, while the poll() code compiled, it didn't actually work
when used.
Rather than reverting the above commit, which would reintroduce
the compile error reporting, we simply undefine the wrappers
so the code calls the WINSOCK native functions once again.
---
lib/poll.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/lib/poll.c b/lib/poll.c
index c4b2127..a26838c 100644
--- a/lib/poll.c
+++ b/lib/poll.c
@@ -54,6 +54,16 @@
#include <sys/select.h>
#include <sys/socket.h>
+#ifdef WINDOWS_NATIVE
+/* The Win32 emulation assumes we're using the Winsock native
+ * functions that accept SOCKETs, not the GNULIB wrapped versions
+ * that accept FDs. The sys/select.h & sys/socket.h header pull
+ * in the wrapped versions, so we must unset them.
+ */
+# undef recv
+# undef select
+#endif
+
#ifdef HAVE_SYS_IOCTL_H
# include <sys/ioctl.h>
#endif

I've just noticed that Bruno pushed a similar patch to git master
yesterday:

commit 4df5fde5d4f69397f3435c9804da5c88ddc7721e
Author: Bruno Haible <***@clisp.org>
Date: Thu May 11 00:38:03 2017 +0200

poll: Fix link error on native Windows.

* lib/poll.c [WINDOWS_NATIVE]: Undefine recv.


unfortunately while that patch may have fixed a linker error, it
does not actually make poll() function correctly. We're still passing
HANDLE objects to the rpl_select() method, instead of calling
the native select() method.

Regards,
Daniel

[Bruno Haible]

Hi Daniel,

it does not actually make poll() function correctly. We're still passing
HANDLE objects to the rpl_select() method, instead of calling
the native select() method.

I don't agree with your reasoning. The select() calls in lib/poll.c:511 and
lib/poll.c:547 is not passing a HANDLE, rather it passes bit masks of FDs
that correspond to SOCKETs. Therefore I propose this patch:


diff --git a/lib/poll.c b/lib/poll.c
index c4b2127..2c6e879 100644
--- a/lib/poll.c
+++ b/lib/poll.c
@@ -80,6 +80,14 @@
first argument, not any possible gnulib override. */
# undef recv

+/* Here it does not matter whether we use the select() function from Windows,
+ that works only when all indicated FDs correspond to sockets and that
+ returns its error code in WSAGetLastError(), or the gnulib override that
+ does not have this limitation and that returns its error code in errno.
+ To simplify testing, use the lower-level Windows select() function
+ always. */
+# undef select
+
static BOOL IsConsoleHandle (HANDLE h)
{
DWORD mode;

[Daniel P. Berrange]

Post by Bruno Haible
Hi Daniel,

it does not actually make poll() function correctly. We're still passing
HANDLE objects to the rpl_select() method, instead of calling
the native select() method.

I don't agree with your reasoning. The select() calls in lib/poll.c:511 and
lib/poll.c:547 is not passing a HANDLE, rather it passes bit masks of FDs

Perhaps I'm mis-understanding, but I was looking at this code:

h = (HANDLE) _get_osfhandle (pfd[i].fd);
assure (h != NULL);
if (IsSocketHandle (h))
{
int requested = FD_CLOSE;

/* see above; socket handles are mapped onto select. */
if (sought & (POLLIN | POLLRDNORM))
{
requested |= FD_READ | FD_ACCEPT;
FD_SET ((SOCKET) h, &rfds);
}
if (sought & (POLLOUT | POLLWRNORM | POLLWRBAND))
{
requested |= FD_WRITE | FD_CONNECT;
FD_SET ((SOCKET) h, &wfds);
}
if (sought & (POLLPRI | POLLRDBAND))
{
requested |= FD_OOB;
FD_SET ((SOCKET) h, &xfds);
}

which takes the 'FD' and gets the associated SOCKET / HANDLE.
The 'xfds' fd_set is thus populated with a SOCKET, not a FD.

A few lines later we call select() with xfds.

The rpl_select() is expecting 'xfds' to be populated with
a FD, not a SOCKET / HANDLE.

Post by Bruno Haible
diff --git a/lib/poll.c b/lib/poll.c
index c4b2127..2c6e879 100644
--- a/lib/poll.c
+++ b/lib/poll.c
@@ -80,6 +80,14 @@
first argument, not any possible gnulib override. */
# undef recv
+/* Here it does not matter whether we use the select() function from Windows,
+ that works only when all indicated FDs correspond to sockets and that
+ returns its error code in WSAGetLastError(), or the gnulib override that
+ does not have this limitation and that returns its error code in errno.
+ To simplify testing, use the lower-level Windows select() function
+ always. */
+# undef select
+
static BOOL IsConsoleHandle (HANDLE h)
{
DWORD mode;

Regards,
Daniel

[Daniel P. Berrange]

Post by Daniel P. Berrange
h = (HANDLE) _get_osfhandle (pfd[i].fd);
assure (h != NULL);
if (IsSocketHandle (h))
{
int requested = FD_CLOSE;
/* see above; socket handles are mapped onto select. */
if (sought & (POLLIN | POLLRDNORM))
{
requested |= FD_READ | FD_ACCEPT;
FD_SET ((SOCKET) h, &rfds);
}
if (sought & (POLLOUT | POLLWRNORM | POLLWRBAND))
{
requested |= FD_WRITE | FD_CONNECT;
FD_SET ((SOCKET) h, &wfds);
}
if (sought & (POLLPRI | POLLRDBAND))
{
requested |= FD_OOB;
FD_SET ((SOCKET) h, &xfds);
}
which takes the 'FD' and gets the associated SOCKET / HANDLE.
The 'xfds' fd_set is thus populated with a SOCKET, not a FD.

Great, thank you for confirming :-)


Regards,
Daniel