Re: wait_reading_process_ouput hangs in certain cases (w/ patches)

[Matthias Dahl <ml_emacs-lists@binary-island.eu>]

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

Hello Eli,

sorry that it has been a while since my last sign of life but 2018 has
been an especially bad year thus far health-wise, so I am struggling
to juggle "everything".

I just wanted to bring the attached fixes back into discussion to get
them into master.

As I understand it, they don't completely fix the problems Andres and
Lars have been seeing, but they still fix real bugs that can cause
random erratic / buggy behavior and/or freezes.

What do you (and all the others in cc' :P) say?

Thanks a lot for taking the time,
Matthias

-- 
Dipl.-Inf. (FH) Matthias Dahl | Software Engineer | binary-island.eu


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-GnuTLS-error-handling.patch --]
[-- Type: text/x-patch; name="0001-Fix-GnuTLS-error-handling.patch", Size: 2146 bytes --]

From b6372518bb174493da482fcfb388aeb8954640dc Mon Sep 17 00:00:00 2001
From: Matthias Dahl <matthias.dahl@binary-island.eu>
Date: Mon, 12 Mar 2018 15:33:45 +0100
Subject: [PATCH 1/3] Fix GnuTLS error handling

* src/gnutls.c (emacs_gnutls_read): All error handling should be done
in `emacs_gnutls_handle_error', move handling of
GNUTLS_E_UNEXPECTED_PACKET_LENGTH accordingly.
We always need to set `errno' in case of an error, since later error
handling (e.g. `wait_reading_process_output') depends on it and GnuTLS
does not set errno by itself. We'll otherwise have random errno values
which can cause erratic behavior and hangs.
(emacs_gnutls_handle_error): GNUTLS_E_UNEXPECTED_PACKET_LENGTH is only
returned for premature connection termination on GnuTLS < 3.0 and is
otherwise a real error and should not be gobbled up.
---
 src/gnutls.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/src/gnutls.c b/src/gnutls.c
index d22d5d267c..5bf5ee0e5c 100644
--- a/src/gnutls.c
+++ b/src/gnutls.c
@@ -708,16 +708,18 @@ emacs_gnutls_read (struct Lisp_Process *proc, char *buf, ptrdiff_t nbyte)
   rtnval = gnutls_record_recv (state, buf, nbyte);
   if (rtnval >= 0)
     return rtnval;
-  else if (rtnval == GNUTLS_E_UNEXPECTED_PACKET_LENGTH)
-    /* The peer closed the connection. */
-    return 0;
   else if (emacs_gnutls_handle_error (state, rtnval))
-    /* non-fatal error */
-    return -1;
-  else {
-    /* a fatal error occurred */
-    return 0;
-  }
+    {
+      /* non-fatal error */
+      errno = EAGAIN;
+      return -1;
+    }
+  else
+    {
+      /* a fatal error occurred */
+      errno = EPROTO;
+      return 0;
+    }
 }
 
 static char const *
@@ -756,8 +758,10 @@ emacs_gnutls_handle_error (gnutls_session_t session, int err)
 	 connection.  */
 # ifdef HAVE_GNUTLS3
       if (err == GNUTLS_E_PREMATURE_TERMINATION)
-	level = 3;
+# else
+      if (err == GNUTLS_E_UNEXPECTED_PACKET_LENGTH)
 # endif
+	level = 3;
 
       GNUTLS_LOG2 (level, max_log_level, "fatal error:", str);
       ret = false;
-- 
2.18.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Always-check-GnuTLS-sessions-for-available-data.patch --]
[-- Type: text/x-patch; name="0002-Always-check-GnuTLS-sessions-for-available-data.patch", Size: 4588 bytes --]

From c52252a862c39b9877ad50640ba6deb352affd11 Mon Sep 17 00:00:00 2001
From: Matthias Dahl <matthias.dahl@binary-island.eu>
Date: Mon, 12 Mar 2018 16:07:55 +0100
Subject: [PATCH 2/3] Always check GnuTLS sessions for available data

* src/process.c (wait_reading_process_output): GnuTLS buffers data
internally and as such there is no guarantee that a select() call on
the underlying kernel socket will report available data if all data
has already been buffered. Prior to GnuTLS < 2.12, lowat mode was the
default which left bytes back in the kernel socket, so a select() call
would work. With GnuTLS >= 2.12 (the now required version for Emacs),
that default changed to non-lowat mode (and we don't set it otherwise)
and was subsequently completely removed with GnuTLS >= 3.0.
So, to properly handle GnuTLS sessions, we need to iterate through all
channels, check for available data manually and set the concerning fds
accordingly. Otherwise we might stall/delay unnecessarily or worse.
This also applies to the !just_wait_proc && wait_proc case, which was
previously handled improperly (only wait_proc was checked) and could
cause problems if sessions did have any dependency on one another
through e.g. higher up program logic and waited for one another.
---
 src/process.c | 77 ++++++++++++++++-----------------------------------
 1 file changed, 24 insertions(+), 53 deletions(-)

diff --git a/src/process.c b/src/process.c
index 6dba218c90..5702408985 100644
--- a/src/process.c
+++ b/src/process.c
@@ -5397,60 +5397,31 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
 #endif	/* !HAVE_GLIB */
 
 #ifdef HAVE_GNUTLS
-          /* GnuTLS buffers data internally.  In lowat mode it leaves
-             some data in the TCP buffers so that select works, but
-             with custom pull/push functions we need to check if some
-             data is available in the buffers manually.  */
-          if (nfds == 0)
+          /* GnuTLS buffers data internally. select() will only report
+             available data for the underlying kernel sockets API, not
+             what has been buffered internally. As such, we need to loop
+             through all channels and check for available data manually.  */
+          if (nfds >= 0)
 	    {
-	      fd_set tls_available;
-	      int set = 0;
-
-	      FD_ZERO (&tls_available);
-	      if (! wait_proc)
-		{
-		  /* We're not waiting on a specific process, so loop
-		     through all the channels and check for data.
-		     This is a workaround needed for some versions of
-		     the gnutls library -- 2.12.14 has been confirmed
-		     to need it.  See
-		     http://comments.gmane.org/gmane.emacs.devel/145074 */
-		  for (channel = 0; channel < FD_SETSIZE; ++channel)
-		    if (! NILP (chan_process[channel]))
-		      {
-			struct Lisp_Process *p =
-			  XPROCESS (chan_process[channel]);
-			if (p && p->gnutls_p && p->gnutls_state
-			    && ((emacs_gnutls_record_check_pending
-				 (p->gnutls_state))
-				> 0))
-			  {
-			    nfds++;
-			    eassert (p->infd == channel);
-			    FD_SET (p->infd, &tls_available);
-			    set++;
-			  }
-		      }
-		}
-	      else
-		{
-		  /* Check this specific channel.  */
-		  if (wait_proc->gnutls_p /* Check for valid process.  */
-		      && wait_proc->gnutls_state
-		      /* Do we have pending data?  */
-		      && ((emacs_gnutls_record_check_pending
-			   (wait_proc->gnutls_state))
-			  > 0))
-		    {
-		      nfds = 1;
-		      eassert (0 <= wait_proc->infd);
-		      /* Set to Available.  */
-		      FD_SET (wait_proc->infd, &tls_available);
-		      set++;
-		    }
-		}
-	      if (set)
-		Available = tls_available;
+              for (channel = 0; channel < FD_SETSIZE; ++channel)
+                if (! NILP (chan_process[channel]))
+                  {
+                    struct Lisp_Process *p =
+                      XPROCESS (chan_process[channel]);
+
+                    if (just_wait_proc && p != wait_proc)
+                      continue;
+
+                    if (p && p->gnutls_p && p->gnutls_state
+                        && ((emacs_gnutls_record_check_pending
+                             (p->gnutls_state))
+                            > 0))
+                      {
+                        nfds++;
+                        eassert (p->infd == channel);
+                        FD_SET (p->infd, &Available);
+                      }
+                  }
 	    }
 #endif
 	}
-- 
2.18.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-Make-xg_select-behave-more-like-pselect.patch --]
[-- Type: text/x-patch; name="0003-Make-xg_select-behave-more-like-pselect.patch", Size: 1322 bytes --]

From 63c4c10b9ef4fb7b40f008db5c9969357312ceae Mon Sep 17 00:00:00 2001
From: Matthias Dahl <matthias.dahl@binary-island.eu>
Date: Tue, 13 Mar 2018 15:35:16 +0100
Subject: [PATCH 3/3] Make xg_select() behave more like pselect()

* src/xgselect.c (xg_select): If no file descriptors have data ready,
pselect() clears the passed in fd sets whereas xg_select() does not
which caused Bug#21337 for `wait_reading_process_output'.
Clear the passed in sets if no fds are ready but leave them untouched
if pselect() returns an error -- just like pselect() does itself.
---
 src/xgselect.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/xgselect.c b/src/xgselect.c
index fedd3127ef..f68982143e 100644
--- a/src/xgselect.c
+++ b/src/xgselect.c
@@ -143,6 +143,14 @@ xg_select (int fds_lim, fd_set *rfds, fd_set *wfds, fd_set *efds,
             ++retval;
         }
     }
+  else if (nfds == 0)
+    {
+      // pselect() clears the file descriptor sets if no fd is ready (but
+      // not if an error occurred), so should we to be compatible. (Bug#21337)
+      if (rfds) FD_ZERO (rfds);
+      if (wfds) FD_ZERO (wfds);
+      if (efds) FD_ZERO (efds);
+    }
 
   /* If Gtk+ is in use eventually gtk_main_iteration will be called,
      unless retval is zero.  */
-- 
2.18.0