GNU bug report logs - #40665
28.0.50; tls hang on local ssl

Previous Next

Full log

Message #92 received at 40665 <at> debbugs.gnu.org (full text, mbox):

From: Derek Zhou <derek <at> 3qin.us>
To: Robert Pluim <rpluim <at> gmail.com>
Cc: 40665 <at> debbugs.gnu.org
Subject: Re: bug#40665: 28.0.50; tls hang on local ssl
Date: Thu, 23 Apr 2020 02:20:53 +0000 (UTC)

[Message part 1 (text/plain, inline)]

Robert Pluim writes:

> OK, that does make sense, and might even be more correct, but itʼs a
> bigger change. You'll need more than just me to agree with it.
>
Patch reworked:

 * before the select, check every interesting gnutls stream for
   available data in the buffer
 * if some of them hit, and either there is no wait_proc or the
   wait_proc is one of the gnutls streams with new data, set the select
   timeout to 0
 * after the select, merge the gnutls buffer status into the select
   returns

The patch is not much longer than before, still a net reduction of code
lines. I've done some light testing and haven't found any problem. 

[check_pending_before_select.patch (text/x-diff, inline)]

diff --git a/src/process.c b/src/process.c
index 91d426103d..783ce098b3 100644
--- a/src/process.c
+++ b/src/process.c
@@ -5497,6 +5497,10 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
 	}
       else
 	{
+#ifdef HAVE_GNUTLS
+	  int tls_nfds;
+	  fd_set tls_available;
+#endif
 	  /* Set the timeout for adaptive read buffering if any
 	     process has non-zero read_output_skip and non-zero
 	     read_output_delay, and we are not reading output for a
@@ -5566,6 +5570,36 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
 	    }
 #endif
 
+#ifdef HAVE_GNUTLS
+          /* GnuTLS buffers data internally. We need to check if some
+	     data is available in the buffers manually before the select.
+	     And if so, we need to skip the select which could block */
+	  FD_ZERO (&tls_available);
+	  tls_nfds = 0;
+	  for (channel = 0; channel < FD_SETSIZE; ++channel)
+	    if (! NILP (chan_process[channel]) && FD_ISSET(channel, &Available))
+	      {
+		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))
+		  {
+		    tls_nfds++;
+		    eassert (p->infd == channel);
+		    FD_SET (p->infd, &tls_available);
+		  }
+	      }
+	  /* if wait_proc is somebody else, we have to wait in select as usual.
+	     Otherwisr, clobber the timeout */
+	  if ((tls_nfds > 0) &&
+	      (!wait_proc ||
+	       (wait_proc->infd >= 0 &&
+		FD_ISSET(wait_proc->infd, &tls_available))))
+	    timeout = make_timespec (0, 0);
+#endif
+
 /* Non-macOS HAVE_GLIB builds call thread_select in xgselect.c.  */
 #if defined HAVE_GLIB && !defined HAVE_NS
 	  nfds = xg_select (max_desc + 1,
@@ -5584,60 +5618,22 @@ 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)
+      /* merge tls_available into Available */
+      if (tls_nfds > 0)
+	{
+	  if (nfds == 0 || (nfds < 0 && errno == EINTR))
 	    {
-	      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.  */
-		  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;
+	      /* fast path, just copy */
+	      nfds = tls_nfds;
+	      Available = tls_available;
 	    }
+	  else if (nfds > 0)
+	    /* slow path, merge one by one.
+	       Note: nfds does not need to be accurate, just positive is enough */
+	    for (channel = 0; channel < FD_SETSIZE; ++channel)
+	      if (FD_ISSET(channel, &tls_available))
+		  FD_SET(channel, &Available);
+	}
 #endif
 	}
 

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.