bug#15648: 24.2.50; gnutls SSL connection to IMAP server causes emacs to crash completely

[Ted Zlatanov <tzz@lifelogs.com>]

On Fri, 25 Oct 2013 21:37:14 +0300 Eli Zaretskii <eliz@gnu.org> wrote: 

>> From: Ted Zlatanov <tzz@lifelogs.com>
>> Cc: emacs@kosowsky.org,  15648@debbugs.gnu.org
>> Date: Fri, 25 Oct 2013 11:38:05 -0400
>> 
EZ> If so, perhaps the problem is that we leave the process object
EZ> marked as a GnuTLS process, but with a NULL state?  Should we remove
EZ> the mark, or maybe delete the process object in gnutls-negotiate?
>> 
>> I would abort with a message like any other error.

EZ> Maybe you should install a change that does that, and see if it solves
EZ> the problem.

You mean like this?  Should I have a special check for "gnutls_p is set
but gnutls_state is NULL"?

=== modified file 'src/process.c'
--- src/process.c       2013-11-04 06:09:03 +0000
+++ src/process.c       2013-11-04 16:43:26 +0000
@@ -4609,7 +4609,7 @@
                      {
                        struct Lisp_Process *p =
                          XPROCESS (chan_process[channel]);
-                       if (p && p->gnutls_p && p->infd
+                       if (p && p->gnutls_p && p->gnutls_state && p->infd
                            && ((emacs_gnutls_record_check_pending
                                 (p->gnutls_state))
                                > 0))
@@ -4623,6 +4623,7 @@
                {
                  /* Check this specific channel. */
                  if (wait_proc->gnutls_p /* Check for valid process.  */
+                      && p->gnutls_state
                      /* Do we have pending data?  */
                      && ((emacs_gnutls_record_check_pending
                           (wait_proc->gnutls_state))
@@ -5004,7 +5005,7 @@
          proc_buffered_char[channel] = -1;
        }
 #ifdef HAVE_GNUTLS
-      if (p->gnutls_p)
+      if (p->gnutls_p && p->gnutls_state)
        nbytes = emacs_gnutls_read (p, chars + carryover + buffered,
                                    readmax - buffered);
       else
@@ -5498,7 +5499,7 @@
 #endif
            {
 #ifdef HAVE_GNUTLS
-             if (p->gnutls_p)
+             if (p->gnutls_p && p->gnutls_state)
                written = emacs_gnutls_write (p, cur_buf, cur_len);
              else
 #endif

EZ> We _begin_ by setting gnutls_state to NULL.  What could possibly
EZ> happen is that we somehow let the process object with a NULL state
EZ> escape from the initialization step, and then
EZ> wait_reading_process_output stumbles on it and tries to use it,
EZ> because the gnutls_p flag is also set right at the beginning.

EZ> How about if we set the gnutls_p flag only when the whole
EZ> initialization succeeds completely?  It's only then that the process
EZ> is ready to be used in conjunction with GnuTLS, isn't it?

You're absolutely right.  Can you check if this patch is good?  It looks
OK to me.

Thank you very much for the guidance.

Ted

=== modified file 'src/gnutls.c'
--- src/gnutls.c        2013-11-04 06:09:03 +0000
+++ src/gnutls.c        2013-11-04 16:36:18 +0000
@@ -810,7 +822,6 @@
   c_hostname = SSDATA (hostname);
 
   state = XPROCESS (proc)->gnutls_state;
-  XPROCESS (proc)->gnutls_p = 1;
 
   if (TYPE_RANGED_INTEGERP (int, loglevel))
     {
@@ -833,7 +844,6 @@
   emacs_gnutls_deinit (proc);
 
   /* Mark PROC as a GnuTLS process.  */
-  XPROCESS (proc)->gnutls_p = 1;
   XPROCESS (proc)->gnutls_state = NULL;
   XPROCESS (proc)->gnutls_x509_cred = NULL;
   XPROCESS (proc)->gnutls_anon_cred = NULL;
@@ -1093,6 +1103,9 @@
       fn_gnutls_x509_crt_deinit (gnutls_verify_cert);
     }
 
+  // Only set this flag if the whole initialization succeeded.
+  XPROCESS (proc)->gnutls_p = 1;
+
   return gnutls_make_error (ret);
 }