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

Previous Next

Full log

Message #104 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: Glenn Morris <rgm <at> gnu.org>, 40665 <at> debbugs.gnu.org,
 Paul Eggert <eggert <at> cs.ucla.edu>, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#40665: 28.0.50; tls hang on local ssl
Date: Fri, 31 Jul 2020 23:22:02 +0000 (UTC)

Robert,

My patch still apply cleanly to either emacs-27 or master branch. I don't know who
else can review this, so I am adding a few random people from the git
log, sorry for the spam. Just don't it to sit in the dust and got forgotten. 

Derek

Derek Zhou writes:

> 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. 
>
> 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.