From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Matthias Dahl Newsgroups: gmane.emacs.devel Subject: Re: wait_reading_process_ouput hangs in certain cases (w/ patches) Date: Tue, 26 Jun 2018 15:36:19 +0200 Message-ID: <03e60e83-c832-ed6b-38bc-c0a20d2a0eac@binary-island.eu> References: <709614e8-1937-07c1-f554-b453ed4f3d4a@binary-island.eu> <7550438b-9fd4-d374-e571-8bb16456cad5@cs.ucla.edu> <797d0e16-1bae-50c2-35f8-05489ffce935@binary-island.eu> <83tvugdiu5.fsf@gnu.org> <877er5s0xv.fsf@gmail.com> <4e4c72bb-295d-81e1-e4ed-cad256bca83c@binary-island.eu> <87zi3v9461.fsf@gmail.com> <87k1uy8x68.fsf@gmail.com> <6d1970af-8c5c-20ba-be09-0b9aa757d663@binary-island.eu> <13b3e003-d12b-33a7-3ebe-c07b017a7cc0@binary-island.eu> <833714rm3d.fsf@gnu.org> <7b64afc3-e9b0-536d-1e42-a5f5d74f1adf@binary-island.eu> <83y3iur4jo.fsf@gnu.org> <2e181861-cfb8-a461-dfb6-9fee2164a611@binary-island.eu> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------2719D444C934FF05696DC529" X-Trace: blaine.gmane.org 1530021876 28097 195.159.176.226 (26 Jun 2018 14:04:36 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Tue, 26 Jun 2018 14:04:36 +0000 (UTC) User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 Cc: eggert@cs.ucla.edu, larsi@gnus.org, rrandresf@gmail.com, Stefan Monnier , emacs-devel@gnu.org To: Eli Zaretskii Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Tue Jun 26 16:04:31 2018 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1fXoa6-0007BU-UG for ged-emacs-devel@m.gmane.org; Tue, 26 Jun 2018 16:04:31 +0200 Original-Received: from localhost ([::1]:52926 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fXocE-0008JY-1D for ged-emacs-devel@m.gmane.org; Tue, 26 Jun 2018 10:06:42 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:37203) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fXoYo-0006tk-K5 for emacs-devel@gnu.org; Tue, 26 Jun 2018 10:03:16 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fXoYi-0002O8-DA for emacs-devel@gnu.org; Tue, 26 Jun 2018 10:03:10 -0400 Original-Received: from ud19.udmedia.de ([194.117.254.59]:47310 helo=mail.ud19.udmedia.de) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1fXoYh-0001fG-QG for emacs-devel@gnu.org; Tue, 26 Jun 2018 10:03:04 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=simple; d=binary-island.eu; h= subject:to:cc:references:from:message-id:date:mime-version :in-reply-to:content-type; s=k1; bh=xNibCOqXIBh2Jg7IBJ+ca9bVG4Dr 2uzCxpUrANB5fb8=; b=Pf9V0IET1I9k7eDzDsOiefbTWQQDpnovHUG6FfEh7ulw lWKzLLECfowii/Bif21zYhjZp9TxbPRFxs8iJLfJ5m0UImT/3JpIxqLLbUY6KuLq l3hCjeIXK2NcdiLyP5bhy5VqPiNTfNC4hfIERJprgTATlF4N8v5Eh6JQyEWtwlE= Original-Received: (qmail 8200 invoked from network); 26 Jun 2018 15:36:19 +0200 Original-Received: by mail.ud19.udmedia.de with ESMTPSA (ECDHE-RSA-AES256-GCM-SHA384 encrypted, authenticated); 26 Jun 2018 15:36:19 +0200 X-UD-Smtp-Session: ud19?126p1@07BKlYtvTNoqAoELxUACNDaqJbnKj9Bf Openpgp: id=1E87ADA02EFE759EFC20B2D1042F47D273AA780C Autocrypt: addr=ml_emacs-lists@binary-island.eu; prefer-encrypt=mutual; keydata= xsFNBFflJwUBEAC9fr9/1xQ6yIjhPy2Rk+bcoBwS99iU1XhW370gsY6Zh+1ZocsQSQ4gskND qgUc5VTI0ZvjlQF/HYYr+OZLibLFCeQ/ywzL6dLD2B7tbLFV3s8kgDwqV/LwzD5xbafIWX1v qqsiFA3Wy3HJxfqzIQneeffB281qs8rj07zihneqHSCowB7Pp3VMjpc9xYvBToNoOMZIkP8r fivMaF+2BvaNYYZVrtTTnqfCrNPmTV0WyRtOm9IGOgEUBuBJ5nJaUc0PY8C/pnXjIhV3Ev2y GDOIEMbARIEBct7wY/P0hvWs/MWlvVB0WgpJhTFXTNwhOHroqQ3HdizwLgMZPycvsuOsS4ZB ly5uhTIGwKstfdluxK4nWHzt2AXRuCOqW2+fklwyVSUWGg0rm6n6HmR3BNnff0AeIqfPR7M5 qCqbgIap7JrsuA8E2C0GP95un+cv1XmzHKaZJqu6y98PshV9ArrnrOE5XvmTwgKbVmzx2JPO 3reVokM5eqG1Xcs9jbQ/h467fXkEh/PpknBDqeEyHtxlq/MUg01Gxx+fHhYIYFGW+/h/wYf9 yS1tQJgs/w5kLhZH0TftlV9e/egI5Oyh0v8Yl8H0TkfkAkMVJHCpICW8EqOWMAGdDpQP50lG mbZ6j39V2DgNl4J7IX5KIDDtc7kTIaudUFCynDp1RITxHRITmwARAQABzS5NYXR0aGlhcyBE YWhsIDxtYXR0aGlhcy5kYWhsQGJpbmFyeS1pc2xhbmQuZXU+wsGBBBMBCgArBQJX5ScFAhsB BQk In-Reply-To: <2e181861-cfb8-a461-dfb6-9fee2164a611@binary-island.eu> Content-Language: en-US X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 194.117.254.59 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.org gmane.emacs.devel:226735 Archived-At: This is a multi-part message in MIME format. --------------2719D444C934FF05696DC529 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit 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 --------------2719D444C934FF05696DC529 Content-Type: text/x-patch; name="0001-Fix-GnuTLS-error-handling.patch" Content-Transfer-Encoding: quoted-printable Content-Disposition: attachment; filename="0001-Fix-GnuTLS-error-handling.patch" =46rom b6372518bb174493da482fcfb388aeb8954640dc Mon Sep 17 00:00:00 2001 From: Matthias Dahl 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 =3D gnutls_record_recv (state, buf, nbyte); if (rtnval >=3D 0) return rtnval; - else if (rtnval =3D=3D 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 =3D EAGAIN; + return -1; + } + else + { + /* a fatal error occurred */ + errno =3D EPROTO; + return 0; + } } =20 static char const * @@ -756,8 +758,10 @@ emacs_gnutls_handle_error (gnutls_session_t session,= int err) connection. */ # ifdef HAVE_GNUTLS3 if (err =3D=3D GNUTLS_E_PREMATURE_TERMINATION) - level =3D 3; +# else + if (err =3D=3D GNUTLS_E_UNEXPECTED_PACKET_LENGTH) # endif + level =3D 3; =20 GNUTLS_LOG2 (level, max_log_level, "fatal error:", str); ret =3D false; --=20 2.18.0 --------------2719D444C934FF05696DC529 Content-Type: text/x-patch; name="0002-Always-check-GnuTLS-sessions-for-available-data.patch" Content-Transfer-Encoding: quoted-printable Content-Disposition: attachment; filename*0="0002-Always-check-GnuTLS-sessions-for-available-data.patch" =46rom c52252a862c39b9877ad50640ba6deb352affd11 Mon Sep 17 00:00:00 2001 From: Matthias Dahl 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 >=3D 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 >=3D 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 */ =20 #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 =3D=3D 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 >=3D 0) { - fd_set tls_available; - int set =3D 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 =3D 0; channel < FD_SETSIZE; ++channel) - if (! NILP (chan_process[channel])) - { - struct Lisp_Process *p =3D - 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 =3D=3D 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 =3D 1; - eassert (0 <=3D wait_proc->infd); - /* Set to Available. */ - FD_SET (wait_proc->infd, &tls_available); - set++; - } - } - if (set) - Available =3D tls_available; + for (channel =3D 0; channel < FD_SETSIZE; ++channel) + if (! NILP (chan_process[channel])) + { + struct Lisp_Process *p =3D + XPROCESS (chan_process[channel]); + + if (just_wait_proc && p !=3D wait_proc) + continue; + + if (p && p->gnutls_p && p->gnutls_state + && ((emacs_gnutls_record_check_pending + (p->gnutls_state)) + > 0)) + { + nfds++; + eassert (p->infd =3D=3D channel); + FD_SET (p->infd, &Available); + } + } } #endif } --=20 2.18.0 --------------2719D444C934FF05696DC529 Content-Type: text/x-patch; name="0003-Make-xg_select-behave-more-like-pselect.patch" Content-Transfer-Encoding: quoted-printable Content-Disposition: attachment; filename="0003-Make-xg_select-behave-more-like-pselect.patch" =46rom 63c4c10b9ef4fb7b40f008db5c9969357312ceae Mon Sep 17 00:00:00 2001 From: Matthias Dahl 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 =3D=3D 0) + { + // pselect() clears the file descriptor sets if no fd is ready (bu= t + // 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); + } =20 /* If Gtk+ is in use eventually gtk_main_iteration will be called, unless retval is zero. */ --=20 2.18.0 --------------2719D444C934FF05696DC529--