bug#23982: [master] Treat errno EINPROGRESS and ENOTCONN as EAGAIN for async connection

bug#23982: [master] Treat errno EINPROGRESS and ENOTCONN as EAGAIN for async connection

[Jun Hao]

[-- Attachment #1: Type: text/plain, Size: 660 bytes --]

Hi,

For latest master branch, when try to use gnutls with async connection,
the handshake will return fatal error if socket is in ENOTCONN or
EINPROGRESS since gnutls treat these errors as fatal during push/pull
and gives up. The later retry will fail because gnutls will mark the
session invalid.

This patch is asking gnutls to treat them as EAGAIN which is non-fatal.

I only tested with OSX. Please see if you can reproduce it on Windows or
Linux and if this patch works for them too.

To reproduce run:

emacs -Q --eval "(erc-tls :server \"irc.freenode.net\" :port 6697 :nick \"test\")"

Current master branch will gave up connecting.

-- 
Thanks - Jun


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Treat-errno-EINPROGRESS-and-ENOTCONN-as-EAGAIN-for-a.patch --]
[-- Type: text/x-patch, Size: 2681 bytes --]

From 8c69cab078d4c51d5c8f76f2aacb7bb8dd46dd7f Mon Sep 17 00:00:00 2001
From: Jun Hao <jun_hao@aol.com>
Date: Thu, 14 Jul 2016 21:47:24 +0800
Subject: [PATCH] Treat errno EINPROGRESS and ENOTCONN as EAGAIN for async
 connection

* src/gnutls.c: (emacs_gnutls_non_blocking_errno): treat errno
EINPROGRESS and ENOTCONN as EAGAIN
(emacs_gnutls_handshake): set errno function to it when async
connection
(Fgnutls_boot): set state with GNUTLS_NONBLOCK flag when async
connection
---
 src/gnutls.c | 36 ++++++++++++++++++++++++++++++++++--
 1 file changed, 34 insertions(+), 2 deletions(-)

diff --git a/src/gnutls.c b/src/gnutls.c
index 7f05ac4..449a971 100644
--- a/src/gnutls.c
+++ b/src/gnutls.c
@@ -35,6 +35,10 @@ along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
 
 static bool emacs_gnutls_handle_error (gnutls_session_t, int);
 
+#ifndef WINDOWSNT
+static int emacs_gnutls_non_blocking_errno(gnutls_transport_ptr_t ptr);
+#endif
+
 static bool gnutls_global_initialized;
 
 static void gnutls_log_function (int, const char *);
@@ -383,6 +387,21 @@ gnutls_log_function2 (int level, const char *string, const char *extra)
   message ("gnutls.c: [%d] %s %s", level, string, extra);
 }
 
+#ifndef WINDOWSNT
+static int
+emacs_gnutls_non_blocking_errno(gnutls_transport_ptr_t ptr)
+{
+  switch (errno)
+    {
+    case EINPROGRESS:
+    case ENOTCONN:
+      return EAGAIN;
+    default:
+      return errno;
+    }
+}
+#endif
+
 int
 gnutls_try_handshake (struct Lisp_Process *proc)
 {
@@ -460,6 +479,11 @@ emacs_gnutls_handshake (struct Lisp_Process *proc)
       gnutls_transport_set_ptr2 (state,
 				 (void *) (intptr_t) proc->infd,
 				 (void *) (intptr_t) proc->outfd);
+      if (proc->is_non_blocking_client)
+        /* for non blocking connection
+           treat EINPROGRESS and ENOTCONN as EAGAIN */
+        gnutls_transport_set_errno_function(state,
+                                            emacs_gnutls_non_blocking_errno);
 #endif
 
       proc->gnutls_initstage = GNUTLS_STAGE_TRANSPORT_POINTERS_SET;
@@ -1596,8 +1620,16 @@ one trustfile (usually a CA bundle).  */)
 
   /* Call gnutls_init here: */
 
-  GNUTLS_LOG (1, max_log_level, "gnutls_init");
-  ret = gnutls_init (&state, GNUTLS_CLIENT);
+  if (XPROCESS (proc)->is_non_blocking_client)
+    {
+      GNUTLS_LOG (1, max_log_level, "gnutls_init with nonblocking");
+      ret = gnutls_init(&state, GNUTLS_CLIENT|GNUTLS_NONBLOCK);
+    }
+  else
+    {
+      GNUTLS_LOG (1, max_log_level, "gnutls_init");
+      ret = gnutls_init (&state, GNUTLS_CLIENT);
+    }
   XPROCESS (proc)->gnutls_state = state;
   if (ret < GNUTLS_E_SUCCESS)
     return gnutls_make_error (ret);
-- 
2.8.0

bug#23982: [master] Treat errno EINPROGRESS and ENOTCONN as EAGAIN for async connection

[npostavs]

Jun Hao <jun_hao@aol.com> writes:
>
> For latest master branch, when try to use gnutls with async connection,
> the handshake will return fatal error if socket is in ENOTCONN or
> EINPROGRESS since gnutls treat these errors as fatal during push/pull
> and gives up. The later retry will fail because gnutls will mark the
> session invalid.
>
> This patch is asking gnutls to treat them as EAGAIN which is non-fatal.
>
> I only tested with OSX. Please see if you can reproduce it on Windows or
> Linux and if this patch works for them too.
>
> To reproduce run:
>
> emacs -Q --eval "(erc-tls :server \"irc.freenode.net\" :port 6697 :nick \"test\")"
>
> Current master branch will gave up connecting.

On both Window and GNU/Linux this successfully connect both with and
without the patch, i.e., patch seems not to be needed on non-OSX
platforms, but it doesn't seem to hurt either.

bug#23982: [master] Treat errno EINPROGRESS and ENOTCONN as EAGAIN for async connection

[Eli Zaretskii]

> From: npostavs@users.sourceforge.net
> Date: Fri, 15 Jul 2016 23:33:00 -0400
> Cc: 23982@debbugs.gnu.org
> 
> > emacs -Q --eval "(erc-tls :server \"irc.freenode.net\" :port 6697 :nick \"test\")"
> >
> > Current master branch will gave up connecting.
> 
> On both Window and GNU/Linux this successfully connect both with and
> without the patch, i.e., patch seems not to be needed on non-OSX
> platforms, but it doesn't seem to hurt either.

I think this fact should be reflected in some comment to the code
changed by the patch.

Thanks.

bug#23982: [master] Treat errno EINPROGRESS and ENOTCONN as EAGAIN for async connection

[jun_hao]

> On Jul 16, 2016, at 11:33, npostavs@users.sourceforge.net wrote:
> 
> Jun Hao <jun_hao@aol.com> writes:
>> 
>> For latest master branch, when try to use gnutls with async connection,
>> the handshake will return fatal error if socket is in ENOTCONN or
>> EINPROGRESS since gnutls treat these errors as fatal during push/pull
>> and gives up. The later retry will fail because gnutls will mark the
>> session invalid.
>> 
>> This patch is asking gnutls to treat them as EAGAIN which is non-fatal.
>> 
>> I only tested with OSX. Please see if you can reproduce it on Windows or
>> Linux and if this patch works for them too.
>> 
>> To reproduce run:
>> 
>> emacs -Q --eval "(erc-tls :server \"irc.freenode.net\" :port 6697 :nick \"test\")"
>> 
>> Current master branch will gave up connecting.
> 
> On both Window and GNU/Linux this successfully connect both with and
> without the patch, i.e., patch seems not to be needed on non-OSX
> platforms, but it doesn't seem to hurt either.

I think it depends on how fast is connect call succeed. If it's already connected before handshake, it'll be okay. The default method that gnutls uses is send(2), which requires the underlying socket to be in connected state. I'm not sure if it's because of OSX or maybe slow internet connection at this point.

Thanks - Jun

bug#23982: [master] Treat errno EINPROGRESS and ENOTCONN as EAGAIN for async connection

[Paul Eggert]

Thanks for the bug report and patch. Two thoughts:

* Passing GNUTLS_NONBLOCK to gnutls_init seems like a worthy change, but 
why does Emacs need to fiddle with an errno function? Surely GnuTLS 
should do the right thing with async connections, without Emacs having 
to insert a shim. Isn't this a bug in GnuTLS?

* The proposed patch uses gnutls_transport_set_errno_function, 
introduced in GnuTLS 2.12.0. Emacs master currently requires only GnuTLS 
2.6.6 or later. Would it be wise to bump the prerequisite version from 
2.6.6 to 2.12.0? I'll CC: this to Ted Zlanatov to see what he thinks.

bug#23982: [master] Treat errno EINPROGRESS and ENOTCONN as EAGAIN for async connection

[Ted Zlatanov]

On Tue, 26 Jul 2016 09:35:04 +0200 Paul Eggert <eggert@cs.ucla.edu> wrote: 

PE> * The proposed patch uses gnutls_transport_set_errno_function, introduced in
PE> GnuTLS 2.12.0. Emacs master currently requires only GnuTLS 2.6.6 or later. Would
PE> it be wise to bump the prerequisite version from 2.6.6 to 2.12.0? I'll CC: this
PE> to Ted Zlanatov to see what he thinks.

2.12.2 was released on 2011-04-08 so I think 5 years is long enough, we
can bump the required version. From a security standpoint I think it's a
good upgrade and the majority of the users will see it as a good change.

If we do, however, we should also review the API changes and see if our
usage in gnutls.c and gnutls.el can be improved or simplified with new
features like `gnutls_transport_set_errno_function'.

Similarly, we should consider migrating to GnuTLS 3.x if anyone is
interested, it should be a fun project (especially if it maintains 2.12
compatibility).

Ted

bug#23982: [master] Treat errno EINPROGRESS and ENOTCONN as EAGAIN for async connection

[Eli Zaretskii]

> Date: Tue, 26 Jul 2016 10:48:28 -0400
> Cc: Jun Hao <jun_hao@aol.com>, 23982@debbugs.gnu.org,
> 	npostavs@users.sourceforge.net
> 
> Similarly, we should consider migrating to GnuTLS 3.x if anyone is
> interested, it should be a fun project (especially if it maintains 2.12
> compatibility).

??? I'm building Emacs with GnuTLS 3.3.11 since many moons ago.

bug#23982: [master] Treat errno EINPROGRESS and ENOTCONN as EAGAIN for async connection

[Ted Zlatanov]

On Tue, 26 Jul 2016 18:08:02 +0300 Eli Zaretskii <eliz@gnu.org> wrote: 

>> Date: Tue, 26 Jul 2016 10:48:28 -0400
>> Cc: Jun Hao <jun_hao@aol.com>, 23982@debbugs.gnu.org,
>> npostavs@users.sourceforge.net
>> 
>> Similarly, we should consider migrating to GnuTLS 3.x if anyone is
>> interested, it should be a fun project (especially if it maintains 2.12
>> compatibility).

EZ> ??? I'm building Emacs with GnuTLS 3.3.11 since many moons ago.

Of course. I meant taking advantage of 3.x features and APIs while
maintaining 2.12 compatibility. It should be an incremental improvement,
theoretically, but since 3.0.1 was released on 2011-08-21 there have
been quite a few changes.

Ted

bug#23982: [master] Treat errno EINPROGRESS and ENOTCONN as EAGAIN for async connection

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 310 bytes --]

Eli Zaretskii wrote:
> I'm building Emacs with GnuTLS 3.3.11 since many moons ago.

OK, I installed the attached patches to GNU Emacs master. The first one requires 
GnuTLS 2.12.2 or later. The second one uses the GNUTLS_NONBLOCK flag, and works 
around the apparent GnuTLS bug with EINPROGRESS and ENOTCONN.


[-- Attachment #2: 0001-Require-GnuTLS-2.12.2-or-later.txt --]
[-- Type: text/plain, Size: 5106 bytes --]

From b283c603b546f678c3eb7d31b06c28b7da285962 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Wed, 3 Aug 2016 04:33:21 -0400
Subject: [PATCH 1/2] Require GnuTLS 2.12.2 or later
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* configure.ac, etc/NEWS: Require GnuTLS 2.12.2 or later,
instead of merely 2.6.6 or later (Bug#23982#20).
* src/gnutls.c (init_gnutls_functions, emacs_gnutls_handshake)
[WINDOWSNT]: Don’t bother with gnutls_check_version or
gnutls_transport_set_lowat, as they are no longer needed now that
we require gnutls 2.12.2 or later.
---
 configure.ac |  2 +-
 etc/NEWS     |  3 +++
 src/gnutls.c | 23 -----------------------
 3 files changed, 4 insertions(+), 24 deletions(-)

diff --git a/configure.ac b/configure.ac
index cca555c..e8aeae2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2748,7 +2748,7 @@ AC_DEFUN
 
 HAVE_GNUTLS=no
 if test "${with_gnutls}" = "yes" ; then
-  EMACS_CHECK_MODULES([LIBGNUTLS], [gnutls >= 2.6.6],
+  EMACS_CHECK_MODULES([LIBGNUTLS], [gnutls >= 2.12.2],
     [HAVE_GNUTLS=yes], [HAVE_GNUTLS=no])
   if test "${HAVE_GNUTLS}" = "yes"; then
     AC_DEFINE(HAVE_GNUTLS, 1, [Define if using GnuTLS.])
diff --git a/etc/NEWS b/etc/NEWS
index 6462eff..fadf4c2 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -28,6 +28,9 @@ otherwise leave it unmarked.
 ** By default libgnutls is now required when building Emacs.
 Use 'configure --with-gnutls=no' to build even when GnuTLS is missing.
 
+** GnuTLS version 2.12.2 or later is now required, instead of merely
+version 2.6.6 or later.
+
 ** The new option 'configure --enable-gcc-warnings=warn-only' causes
 GCC to issue warnings without stopping the build.  This behavior is
 now the default in developer builds.  As before, use
diff --git a/src/gnutls.c b/src/gnutls.c
index 7f05ac4..681e298 100644
--- a/src/gnutls.c
+++ b/src/gnutls.c
@@ -110,8 +110,6 @@ DEF_DLL_FN (ssize_t, gnutls_record_send,
 	    (gnutls_session_t, const void *, size_t));
 DEF_DLL_FN (const char *, gnutls_strerror, (int));
 DEF_DLL_FN (void, gnutls_transport_set_errno, (gnutls_session_t, int));
-DEF_DLL_FN (const char *, gnutls_check_version, (const char *));
-DEF_DLL_FN (void, gnutls_transport_set_lowat, (gnutls_session_t, int));
 DEF_DLL_FN (void, gnutls_transport_set_ptr2,
 	    (gnutls_session_t, gnutls_transport_ptr_t,
 	     gnutls_transport_ptr_t));
@@ -225,11 +223,6 @@ init_gnutls_functions (void)
   LOAD_DLL_FN (library, gnutls_record_send);
   LOAD_DLL_FN (library, gnutls_strerror);
   LOAD_DLL_FN (library, gnutls_transport_set_errno);
-  LOAD_DLL_FN (library, gnutls_check_version);
-  /* We don't need to call gnutls_transport_set_lowat in GnuTLS 2.11.1
-     and later, and the function was removed entirely in 3.0.0.  */
-  if (!fn_gnutls_check_version ("2.11.1"))
-    LOAD_DLL_FN (library, gnutls_transport_set_lowat);
   LOAD_DLL_FN (library, gnutls_transport_set_ptr2);
   LOAD_DLL_FN (library, gnutls_transport_set_pull_function);
   LOAD_DLL_FN (library, gnutls_transport_set_push_function);
@@ -290,7 +283,6 @@ init_gnutls_functions (void)
 # define gnutls_certificate_set_x509_trust_file fn_gnutls_certificate_set_x509_trust_file
 # define gnutls_certificate_type_get fn_gnutls_certificate_type_get
 # define gnutls_certificate_verify_peers2 fn_gnutls_certificate_verify_peers2
-# define gnutls_check_version fn_gnutls_check_version
 # define gnutls_cipher_get fn_gnutls_cipher_get
 # define gnutls_cipher_get_name fn_gnutls_cipher_get_name
 # define gnutls_credentials_set fn_gnutls_credentials_set
@@ -321,7 +313,6 @@ init_gnutls_functions (void)
 # define gnutls_sign_get_name fn_gnutls_sign_get_name
 # define gnutls_strerror fn_gnutls_strerror
 # define gnutls_transport_set_errno fn_gnutls_transport_set_errno
-# define gnutls_transport_set_lowat fn_gnutls_transport_set_lowat
 # define gnutls_transport_set_ptr2 fn_gnutls_transport_set_ptr2
 # define gnutls_transport_set_pull_function fn_gnutls_transport_set_pull_function
 # define gnutls_transport_set_push_function fn_gnutls_transport_set_push_function
@@ -439,20 +430,6 @@ emacs_gnutls_handshake (struct Lisp_Process *proc)
 				 (gnutls_transport_ptr_t) proc);
       gnutls_transport_set_push_function (state, &emacs_gnutls_push);
       gnutls_transport_set_pull_function (state, &emacs_gnutls_pull);
-
-      /* For non blocking sockets or other custom made pull/push
-	 functions the gnutls_transport_set_lowat must be called, with
-	 a zero low water mark value. (GnuTLS 2.10.4 documentation)
-
-	 (Note: this is probably not strictly necessary as the lowat
-	  value is only used when no custom pull/push functions are
-	  set.)  */
-      /* According to GnuTLS NEWS file, lowat level has been set to
-	 zero by default in version 2.11.1, and the function
-	 gnutls_transport_set_lowat was removed from the library in
-	 version 2.99.0.  */
-      if (!gnutls_check_version ("2.11.1"))
-	gnutls_transport_set_lowat (state, 0);
 #else
       /* This is how GnuTLS takes sockets: as file descriptors passed
 	 in.  For an Emacs process socket, infd and outfd are the
-- 
2.5.5


[-- Attachment #3: 0002-Fix-non-blocking-GnuTLS-with-slow-connection.txt --]
[-- Type: text/plain, Size: 2226 bytes --]

From 3029f74ad5b8f989e26673a31c257fa2aa8d7304 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Wed, 3 Aug 2016 01:54:20 -0700
Subject: [PATCH 2/2] Fix non-blocking GnuTLS with slow connection

Although the problem is reported for OS X (Bug#23982), it seems to
be possible on other POSIXish platforms.
* src/gnutls.c (emacs_gnutls_nonblock_errno) [!WINDOWSNT]:
New function.
(emacs_gnutls_handshake) [!WINDOWSNT]:
Use it as the errno function, if non-blocking.
(Fgnutls_boot): Use GNUTLS_NONBLOCK if non-blocking.
---
 src/gnutls.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/src/gnutls.c b/src/gnutls.c
index 681e298..e3e9311 100644
--- a/src/gnutls.c
+++ b/src/gnutls.c
@@ -411,6 +411,31 @@ gnutls_try_handshake (struct Lisp_Process *proc)
   return ret;
 }
 
+#ifndef WINDOWSNT
+static int
+emacs_gnutls_nonblock_errno (gnutls_transport_ptr_t ptr)
+{
+  int err = errno;
+
+  switch (err)
+    {
+# ifdef _AIX
+      /* This is taken from the GnuTLS system_errno function circa 2016;
+	 see <http://savannah.gnu.org/support/?107464>.  */
+    case 0:
+      errno = EAGAIN;
+      /* Fall through.  */
+# endif
+    case EINPROGRESS:
+    case ENOTCONN:
+      return EAGAIN;
+
+    default:
+      return err;
+    }
+}
+#endif
+
 static int
 emacs_gnutls_handshake (struct Lisp_Process *proc)
 {
@@ -437,6 +462,9 @@ emacs_gnutls_handshake (struct Lisp_Process *proc)
       gnutls_transport_set_ptr2 (state,
 				 (void *) (intptr_t) proc->infd,
 				 (void *) (intptr_t) proc->outfd);
+      if (proc->is_non_blocking_client)
+	gnutls_transport_set_errno_function (state,
+					     emacs_gnutls_nonblock_errno);
 #endif
 
       proc->gnutls_initstage = GNUTLS_STAGE_TRANSPORT_POINTERS_SET;
@@ -1574,7 +1602,8 @@ one trustfile (usually a CA bundle).  */)
   /* Call gnutls_init here: */
 
   GNUTLS_LOG (1, max_log_level, "gnutls_init");
-  ret = gnutls_init (&state, GNUTLS_CLIENT);
+  int nonblock = XPROCESS (proc)->is_non_blocking_client ? GNUTLS_NONBLOCK : 0;
+  ret = gnutls_init (&state, GNUTLS_CLIENT | nonblock);
   XPROCESS (proc)->gnutls_state = state;
   if (ret < GNUTLS_E_SUCCESS)
     return gnutls_make_error (ret);
-- 
2.5.5

bug#23982: [master] Treat errno EINPROGRESS and ENOTCONN as EAGAIN for async connection

[Ted Zlatanov]

On Wed, 3 Aug 2016 02:02:11 -0700 Paul Eggert <eggert@cs.ucla.edu> wrote: 

PE> Eli Zaretskii wrote:
>> I'm building Emacs with GnuTLS 3.3.11 since many moons ago.

PE> OK, I installed the attached patches to GNU Emacs master. The first one requires
PE> GnuTLS 2.12.2 or later. The second one uses the GNUTLS_NONBLOCK flag, and works
PE> around the apparent GnuTLS bug with EINPROGRESS and ENOTCONN.

Thank you!

Ted