From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Lars Ingebrigtsen Newsgroups: gmane.emacs.bugs Subject: bug#40665: 28.0.50; tls hang on local ssl Date: Sun, 02 Aug 2020 07:34:56 +0200 Message-ID: <87ft95objz.fsf@gnus.org> References: <86wo6fo78r.fsf@mail.3qin.us> <86eeslecnf.fsf@mail.3qin.us> <86blnnebh3.fsf@mail.3qin.us> <86zhb5hecx.fsf@mail.3qin.us> <86eeshpqdb.fsf@mail.3qin.us> <86zhb5q7sw.fsf@mail.3qin.us> <86y2qorj76.fsf@mail.3qin.us> <86368w1tge.fsf@mail.3qin.us> <864ktcfpm5.fsf@mail.3qin.us> <86368wfp6d.fsf@mail.3qin.us> <86y2qniu5m.fsf@mail.3qin.us> <86wo67im9w.fsf@mail.3qin.us> <86y2qnaqnv.fsf@mail.3qin.us> Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="31141"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.0.50 (gnu/linux) Cc: Robert Pluim , 40665@debbugs.gnu.org To: Derek Zhou Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Sun Aug 02 07:36:12 2020 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1k26fK-0007zr-RB for geb-bug-gnu-emacs@m.gmane-mx.org; Sun, 02 Aug 2020 07:36:10 +0200 Original-Received: from localhost ([::1]:34532 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1k26fJ-0000k9-An for geb-bug-gnu-emacs@m.gmane-mx.org; Sun, 02 Aug 2020 01:36:09 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:33218) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1k26fC-0000jz-LZ for bug-gnu-emacs@gnu.org; Sun, 02 Aug 2020 01:36:02 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:58128) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1k26fC-00072d-Bi for bug-gnu-emacs@gnu.org; Sun, 02 Aug 2020 01:36:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1k26fC-0003hA-4K for bug-gnu-emacs@gnu.org; Sun, 02 Aug 2020 01:36:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Lars Ingebrigtsen Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sun, 02 Aug 2020 05:36:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 40665 X-GNU-PR-Package: emacs Original-Received: via spool by 40665-submit@debbugs.gnu.org id=B40665.159634651214128 (code B ref 40665); Sun, 02 Aug 2020 05:36:02 +0000 Original-Received: (at 40665) by debbugs.gnu.org; 2 Aug 2020 05:35:12 +0000 Original-Received: from localhost ([127.0.0.1]:41441 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1k26eO-0003fn-Et for submit@debbugs.gnu.org; Sun, 02 Aug 2020 01:35:12 -0400 Original-Received: from quimby.gnus.org ([95.216.78.240]:57878) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1k26eJ-0003fF-PD for 40665@debbugs.gnu.org; Sun, 02 Aug 2020 01:35:11 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnus.org; s=20200322; h=Content-Type:MIME-Version:Message-ID:In-Reply-To:Date: References:Subject:Cc:To:From:Sender:Reply-To:Content-Transfer-Encoding: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=NIeE4Qp1c/wbJRPSurhYwEncvRF64ePiK6OVL25fQZM=; b=goLm0jPKtrfu8yDY9Ev/W9MV/W Bb/5swqkDIr8xpPtDiGp/T9YaLABKfJM5yyH0aKwpZW6JVo0Vd6HHr0jJcRmWHK7d8zTn+FQSNUA1 nljaHdy/VnEWrOWcrcyiHIxUiHQ2XfjlnSaVGHnhiEbFKAnqrGUBklmK2ZCY8a6pxslY=; Original-Received: from cm-84.212.202.86.getinternet.no ([84.212.202.86] helo=xo) by quimby with esmtpsa (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1k26e9-00059m-UM; Sun, 02 Aug 2020 07:35:00 +0200 In-Reply-To: <86y2qnaqnv.fsf@mail.3qin.us> (Derek Zhou's message of "Thu, 23 Apr 2020 02:20:53 +0000 (UTC)") X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.io gmane.emacs.bugs:183807 Archived-At: Derek Zhou writes: > 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. I think the reasoning sounds correct, and I'm now testing it in this Emacs, and I'm not seeing any regressions. I'll test it a bit more, and if I can find no problems, I'll apply your patch to Emacs 28. I've touched up your patch lightly for Emacs coding style (capitalised the first word in the comments etc), and I've also removed some parentheses that seemed superfluous. I've attached the resulting patch below for reference. diff --git a/src/process.c b/src/process.c index 6e5bcf307a..15634e4a8b 100644 --- a/src/process.c +++ b/src/process.c @@ -5491,6 +5491,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 @@ -5560,7 +5564,36 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd, } #endif -/* Non-macOS HAVE_GLIB builds call thread_select in xgselect.c. */ +#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. Otherwise, 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, &Available, (check_write ? &Writeok : 0), @@ -5578,59 +5611,21 @@ 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) { - fd_set tls_available; - int set = 0; - - FD_ZERO (&tls_available); - if (! wait_proc) + if (nfds == 0 || (nfds < 0 && errno == EINTR)) { - /* 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++; - } + /* Fast path, just copy. */ + nfds = tls_nfds; + Available = tls_available; } - if (set) - 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 } -- (domestic pets only, the antidote for overdose, milk.) bloggy blog: http://lars.ingebrigtsen.no