proposed patch to fix GnuTLS handshake bug (bug#11143 and bug#10904)

proposed patch to fix GnuTLS handshake bug (bug#11143 and bug#10904)

[Ted Zlatanov]

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

Several users have reported handshake lockups with GnuTLS due to network
problems or other issues, specifically bug#11143 and bug#10904.  The
attached patch adds a per-connection handshake counter and stops
retrying if the counter hits 100.  It logs both the retries (at level 5,
so users won't see it normally) and the surrender at level 2.

I think it's worth putting into 24.1 despite the code freeze.  I think
it's low-risk and potentially very good for the Emacs users.  The fault
for delaying the bug fix until now is entirely mine; sorry about that.

Ted


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gnutls-handshakes.patch --]
[-- Type: text/x-diff, Size: 2635 bytes --]

=== modified file 'src/gnutls.c'
--- src/gnutls.c	2012-02-13 20:39:46 +0000
+++ src/gnutls.c	2012-04-09 00:34:32 +0000
@@ -259,6 +259,12 @@
   message ("gnutls.c: [%d] %s %s", level, string, extra);
 }
 
+static void
+gnutls_log_function2i (int level, const char* string, int extra)
+{
+  message ("gnutls.c: [%d] %s %d", level, string, extra);
+}
+
 static int
 emacs_gnutls_handshake (struct Lisp_Process *proc)
 {
@@ -399,10 +405,22 @@
   ssize_t rtnval;
   gnutls_session_t state = proc->gnutls_state;
 
+  int log_level = proc->gnutls_log_level;
+
   if (proc->gnutls_initstage != GNUTLS_STAGE_READY)
     {
-      emacs_gnutls_handshake (proc);
-      return -1;
+      if (proc->gnutls_handshakes_tried < GNUTLS_EMACS_HANDSHAKES_LIMIT)
+        {
+          proc->gnutls_handshakes_tried++;
+          emacs_gnutls_handshake (proc);
+          GNUTLS_LOG2i (5, log_level, "Retried handshake", 
+                        proc->gnutls_handshakes_tried);
+          return -1;
+        }
+
+      GNUTLS_LOG (2, log_level, "Giving up on handshake; resetting retries");
+      proc->gnutls_handshakes_tried = 0;
+      return 0;
     }
   rtnval = fn_gnutls_record_recv (state, buf, nbyte);
   if (rtnval >= 0)

=== modified file 'src/gnutls.h'
--- src/gnutls.h	2012-01-05 09:46:05 +0000
+++ src/gnutls.h	2012-04-09 00:29:27 +0000
@@ -23,6 +23,8 @@
 #include <gnutls/gnutls.h>
 #include <gnutls/x509.h>
 
+#define GNUTLS_EMACS_HANDSHAKES_LIMIT 100
+
 typedef enum
 {
   /* Initialization stages.  */
@@ -53,6 +55,8 @@
 
 #define GNUTLS_LOG2(level, max, string, extra) do { if (level <= max) { gnutls_log_function2 (level, "(Emacs) " string, extra); } } while (0)
 
+#define GNUTLS_LOG2i(level, max, string, extra) do { if (level <= max) { gnutls_log_function2i (level, "(Emacs) " string, extra); } } while (0)
+
 extern EMACS_INT
 emacs_gnutls_write (struct Lisp_Process *proc, const char *buf, EMACS_INT nbyte);
 extern EMACS_INT

=== modified file 'src/process.c'
--- src/process.c	2012-03-23 12:23:14 +0000
+++ src/process.c	2012-04-09 00:24:07 +0000
@@ -641,6 +641,7 @@
 #ifdef HAVE_GNUTLS
   p->gnutls_initstage = GNUTLS_STAGE_EMPTY;
   p->gnutls_log_level = 0;
+  p->gnutls_handshakes_tried = 0;
   p->gnutls_p = 0;
   p->gnutls_state = NULL;
   p->gnutls_x509_cred = NULL;

=== modified file 'src/process.h'
--- src/process.h	2012-01-19 07:21:25 +0000
+++ src/process.h	2012-04-09 00:23:24 +0000
@@ -134,6 +134,7 @@
     gnutls_certificate_client_credentials gnutls_x509_cred;
     gnutls_anon_client_credentials_t gnutls_anon_cred;
     int gnutls_log_level;
+    int gnutls_handshakes_tried;
     int gnutls_p;
 #endif
 };

Re: proposed patch to fix GnuTLS handshake bug (bug#11143 and bug#10904)

[Stefan Monnier]

> I think it's worth putting into 24.1 despite the code freeze.  I think
> it's low-risk and potentially very good for the Emacs users.  The fault
> for delaying the bug fix until now is entirely mine; sorry about that.

The patch looks acceptable, but please add some comment to it,
explaining why retries are needed and why they need to be bounded.


        Stefan

Re: proposed patch to fix GnuTLS handshake bug (bug#11143 and bug#10904)

[Ted Zlatanov]

On Sun, 08 Apr 2012 23:02:16 -0400 Stefan Monnier <monnier@iro.umontreal.ca> wrote: 

>> I think it's worth putting into 24.1 despite the code freeze.  I think
>> it's low-risk and potentially very good for the Emacs users.  The fault
>> for delaying the bug fix until now is entirely mine; sorry about that.

SM> The patch looks acceptable, but please add some comment to it,
SM> explaining why retries are needed and why they need to be bounded.

I did and it's in trunk.  Please look it over and consider it for 24.1.

Thanks
Ted

Re: proposed patch to fix GnuTLS handshake bug (bug#11143 and bug#10904)

[Chong Yidong]

Ted Zlatanov <tzz@lifelogs.com> writes:

> I did and it's in trunk.  Please look it over and consider it for 24.1.

Thanks.  It's in emacs-24 now.

Re: proposed patch to fix GnuTLS handshake bug (bug#11143 and bug#10904)

[Ted Zlatanov]

On Tue, 10 Apr 2012 16:01:25 +0800 Chong Yidong <cyd@gnu.org> wrote: 

CY> Ted Zlatanov <tzz@lifelogs.com> writes:
>> I did and it's in trunk.  Please look it over and consider it for 24.1.

CY> Thanks.  It's in emacs-24 now.

Thomas Fitzsimmons confirmed the patch fixed bug#10904, good news.
There's some more improvement needed to fall back from NORMAL to
PERFORMANCE priority string if the connection fails to negotiate, but
that's not urgent.

Ted