bug#45821: Emacs UDP support on Windows

[Robert Pluim <rpluim@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2707 bytes --]

>>>>> On Mon, 2 Jan 2023 22:56:04 +0000, Alex Matei <matei.alexandru@live.com> said:

    Alex> Thanks for the tips with logging!
    Alex> I think I found the fix (at least for one of the issues?)
    Alex> Please see the attached patch/diff for the fix.

    Alex> The code will always ignore (aka ‘eat’) one character if the file
    Alex> descriptor had the least significant bit set (0x1, so this would mean
    Alex> for any file descriptor with READ flag set)
    Alex> [cid:image004.png@01D91EBA.5498F140]

Hmm, my local version already had that fixed, so I guess I posted the
wrong patch to the bug. But I still think you found a fix (see below)

    Alex> Based on operator precedence rules, FILE_SOCKET == 0 will be evaluated
    Alex> first (before &) , resulting in 0, which would effectively cause the
    Alex> condition to always be false

    Alex> This behavior can be observed when executing a call to
    Alex> ‘(async-shell-command)’ (-> file descriptor flags 0x191) on a build
    Alex> with the patch vs one without the patch applied (see below, attached
    Alex> image)

    Alex>   * My suspicion is that this behavior is similar to what TLS was
    Alex> experiencing but just easier to reproduce (note: there might other
    Alex> issues with TLS , but so far, after applying this fix I have not run
    Alex> into any other issues)

    Alex> [cid:image005.png@01D91EBA.5498F140]


    Alex> @Robert Pluim<mailto:rpluim@gmail.com> I don’t know if this was the
    Alex> only missing piece, but from all my tests I couldn’t see any issues
    Alex> with TLS anymore, and with navigating https://gnu.org in EWW..
    Alex> I wanted to thank you for creating this patch 🙏, and giving me pointers on how to apply and debug it.

Iʼd run the networking portion of our test suite to see if everything
works as intended (you might need to install the GnuTLS cli tools). eg:

    cd test
    make network-stream-tests
    
    Alex> p.s.
    Alex> - The patch also adds STATUS_READ_IN_PROGRESS state for the new
    Alex> ‘_sys_wait_readable’ function , based on what the read ahead logic was
    Alex> doing already (idk if it is needed, but it is a nice mirror, and a
    Alex> good status code to reflect, although I am not convinced anyone cares
    Alex> about this state transition..)

I think this is actually the fix, since I added it locally and TLS
started working, but I donʼt understand the code enough to be definite
about that. More testing required 😀

Can you post the full patch just to ensure weʼre all talking about the
same changes? Iʼve attached what Iʼm working from

Robert
-- 


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Make-UDP-sockets-work-on-Windows.patch --]
[-- Type: text/x-diff, Size: 5046 bytes --]

From 2ae91ed495f3972ddac383bd5f63c47946d5cdb5 Mon Sep 17 00:00:00 2001
From: Robert Pluim <rpluim@gmail.com>
Date: Mon, 27 Sep 2021 17:25:27 +0200
Subject: [PATCH] Make UDP sockets work on Windows
To: emacs-devel@gnu.org

* admin/CPP-DEFINES: remove BROKEN_DATAGRAM_SOCKETS
* nt/inc/ms-w32.h: remove BROKEN_DATAGRAM_SOCKETS.
* src/process.c (DATAGRAM_SOCKETS): remove dependency on
!BROKEN_DATAGRAM_SOCKETS.
* src/w32.c (_sys_wait_readable): New function.  Calls
pfn_WSAEventSelect for sockets to see if data can be read, so the
1-byte readahead is no longer necessary.
(sys_read): Only do 1-byte readahead for non-sockets.
* src/w32.h: Add prototype for _sys_wait_readable.
* src/w32proc.c (reader_thread): Call _sys_wait_readable for socket
handles.
---
 admin/CPP-DEFINES |  1 -
 nt/inc/ms-w32.h   |  4 ----
 src/process.c     |  8 +++-----
 src/w32.c         | 49 +++++++++++++++++++++++++++++++++++++++++++----
 src/w32.h         |  1 +
 src/w32proc.c     |  2 ++
 6 files changed, 51 insertions(+), 14 deletions(-)

diff --git a/admin/CPP-DEFINES b/admin/CPP-DEFINES
index 06986ec8f48..2704bc57675 100644
--- a/admin/CPP-DEFINES
+++ b/admin/CPP-DEFINES
@@ -103,7 +103,6 @@ getting at the full user name.  Only MSDOS overrides the default.
 anymore, so they can be removed.
 
 AMPERSAND_FULL_NAME
-BROKEN_DATAGRAM_SOCKETS
 BROKEN_GET_CURRENT_DIR_NAME
 BROKEN_PTY_READ_AFTER_EAGAIN
 DEFAULT_SOUND_DEVICE
diff --git a/nt/inc/ms-w32.h b/nt/inc/ms-w32.h
index 58be1199345..535d24cd57b 100644
--- a/nt/inc/ms-w32.h
+++ b/nt/inc/ms-w32.h
@@ -89,10 +89,6 @@ #define _CALLBACK_ __cdecl
    Look in <sys/time.h> for a timeval structure.  */
 #define HAVE_TIMEVAL 1
 
-/* Our select emulation does 1-byte read-ahead waiting for received
-   packets, so datagrams are broken.  */
-#define BROKEN_DATAGRAM_SOCKETS 1
-
 #define MAIL_USE_SYSTEM_LOCK 1
 
 /* Define to 1 if GCC-style __attribute__ ((__aligned__ (expr))) works. */
diff --git a/src/process.c b/src/process.c
index 67d1d3e425f..8f7fccfe538 100644
--- a/src/process.c
+++ b/src/process.c
@@ -234,11 +234,9 @@ #define PIPECONN1_P(p) (EQ (p->type, Qpipe))
    "non-destructive" select.  So we require either native select,
    or emulation of select using FIONREAD.  */
 
-#ifndef BROKEN_DATAGRAM_SOCKETS
-# if defined HAVE_SELECT || defined USABLE_FIONREAD
-#  if defined HAVE_SENDTO && defined HAVE_RECVFROM && defined EMSGSIZE
-#   define DATAGRAM_SOCKETS
-#  endif
+#if defined HAVE_SELECT || defined USABLE_FIONREAD
+# if defined HAVE_SENDTO && defined HAVE_RECVFROM && defined EMSGSIZE
+#  define DATAGRAM_SOCKETS
 # endif
 #endif
 
diff --git a/src/w32.c b/src/w32.c
index 47d79abc5b0..ef7d3de861b 100644
--- a/src/w32.c
+++ b/src/w32.c
@@ -8940,6 +8940,43 @@ _sys_wait_accept (int fd)
   return cp->status;
 }
 
+int
+_sys_wait_readable (int fd)
+{
+  HANDLE hEv;
+  child_process * cp;
+  int rc;
+
+  if (fd < 0 || fd >= MAXDESC)
+    return STATUS_READ_ERROR;
+
+  cp = fd_info[fd].cp;
+
+  if (cp == NULL || cp->fd != fd || cp->status != STATUS_READ_READY)
+    return STATUS_READ_ERROR;
+
+  cp->status = STATUS_READ_FAILED;
+
+  hEv = pfn_WSACreateEvent ();
+  rc = pfn_WSAEventSelect (SOCK_HANDLE (fd), hEv, FD_READ);
+  if (rc != SOCKET_ERROR)
+    {
+      do
+        {
+          rc = WaitForSingleObject (hEv, 500);
+          Sleep (5);
+        } while (rc == WAIT_TIMEOUT
+                 && cp->status != STATUS_READ_ERROR
+                 && cp->char_avail);
+      pfn_WSAEventSelect (SOCK_HANDLE (fd), NULL, 0);
+      if (rc == WAIT_OBJECT_0)
+        cp->status = STATUS_READ_SUCCEEDED;
+    }
+  pfn_WSACloseEvent (hEv);
+
+  return cp->status;
+}
+
 int
 _sys_wait_connect (int fd)
 {
@@ -9065,10 +9102,14 @@ sys_read (int fd, char * buffer, unsigned int count)
 	      return -1;
 
 	    case STATUS_READ_SUCCEEDED:
-	      /* consume read-ahead char */
-	      *buffer++ = cp->chr;
-	      count--;
-	      nchars++;
+              /* select on sockets no longer requires a 1-byte read.  */
+              if ((fd_info[fd].flags & FILE_SOCKET) == 0)
+		{
+		  /* consume read-ahead char */
+		  *buffer++ = cp->chr;
+		  count--;
+		  nchars++;
+		}
 	      cp->status = STATUS_READ_ACKNOWLEDGED;
 	      ResetEvent (cp->char_avail);
 
diff --git a/src/w32.h b/src/w32.h
index a3d0b75359a..4eba0fabe94 100644
--- a/src/w32.h
+++ b/src/w32.h
@@ -177,6 +177,7 @@ #define FILE_DONT_CLOSE         0x1000
 
 extern int _sys_read_ahead (int fd);
 extern int _sys_wait_accept (int fd);
+extern int _sys_wait_readable (int fd);
 extern int _sys_wait_connect (int fd);
 
 extern HMODULE w32_delayed_load (Lisp_Object);
diff --git a/src/w32proc.c b/src/w32proc.c
index 77a4ac1ff7e..19d9fc7d0e7 100644
--- a/src/w32proc.c
+++ b/src/w32proc.c
@@ -1243,6 +1243,8 @@ reader_thread (void *arg)
 	rc = _sys_wait_connect (fd);
       else if (fd >= 0 && (fd_info[fd].flags & FILE_LISTEN) != 0)
 	rc = _sys_wait_accept (fd);
+      else if (fd_info[fd].flags & FILE_SOCKET)
+	rc = _sys_wait_readable (fd);
       else
 	rc = _sys_read_ahead (fd);
 
-- 
2.38.1.420.g319605f8f0