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: 1575 bytes --]

Hello all...

Sorry for the delay, things took quite a bit longer than I expected. And
I really should scratch that "later this week" phrase entirely from my
active use. ;-)

@Andrés: Sorry for not replying to your off-list mail yet. I was busy
with this bug, given the limited time I have I was prioritizing.

So, attached you will find patches that fix bugs I have found with the
current GnuTLS code. I am pretty confident that it will fix the issue
that Lars is seeing. And I hope that the hangs seen by Andrés will be
gone with this as well.

Lars and Andrés, please test those patches either against Emacs master
without any other additional patches or against the current Emacs 26
branch with the wait_reading_process_output patches that have been
applied to master but nothing else.

Please report back if those patches fix your issues or, if not, how they
affect (if at all) the hangs. Thanks again for investing the time!

@Eli: Those patches are, imho, clearly Emacs 26 material as well. And
along those lines, and I don't mean to be pushy at all :), I would like
to bring up the wait_reading_process_output fixes as well. I still think
it would be a good idea to get those into Emacs 26. All the fixes up
until now have been for hangs that will usually be erratic and
unpredictable to the user... thus, those hangs will probably end up
as unresolved bug reports on some random package instead of posts on
this list... if at all.

Keeping my fingers crossed here. ;-)

So long,
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 841d0fbe37642bc14c4bcd515cfbc91a25ba179b 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/2] 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 903393fed1..e7d0d3d845 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.16.2


[-- 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: 4596 bytes --]

From f5b979da67595da4d91f7b7a4eb979deae6a7321 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/2] 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 9b9b9f3550..f8fed56d5b 100644
--- a/src/process.c
+++ b/src/process.c
@@ -5392,60 +5392,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.16.2