Fix a hang in gpgme.
authorWerner Koch <wk@gnupg.org>
Wed, 8 May 2013 18:54:49 +0000 (20:54 +0200)
committerWerner Koch <wk@gnupg.org>
Wed, 8 May 2013 18:54:49 +0000 (20:54 +0200)
* patches/gpgme-1.4.1/01-improve-trace.patch: New.
* patches/gpgme-1.4.1/02-close-socket.patch:
* Makefile.am (EXTRA_DIST): Add patches.
--

When using a socket (e.g. polling scdaemon) the closing of the socket
often blocked waiting on termination of the reader thread.  At that
point the reader table lock is also held and thus no other reader
thread could be started.  See the comments in the code for out
solution.

Note that there is currently another hang in Kleopatra, the first time
it is started. I was able to workaround that by disabling the startup
selftests.

Makefile.am
patches/gpgme-1.4.1/01-improve-trace.patch [new file with mode: 0755]
patches/gpgme-1.4.1/02-close-socket.patch [new file with mode: 0755]

index b3f92ec..5c25df2 100644 (file)
@@ -43,6 +43,8 @@ EXTRA_DIST = autogen.sh README.GIT ONEWS \
        patches/gpgme-1.2.0/03-w32-socket.patch \
         patches/gpgme-1.2.0/04-check-agent.patch \
         patches/gpgme-1.2.0/05-is-socket.patch \
+        patches/gpgme-1.4.1/01-improve-trace.patch \
+        patches/gpgme-1.4.1/02-close-socket.patch \
        patches/gpgex-0.9.5/01-default-no-suffix.patch \
        patches/gnupg2/01-version.patch.in \
        patches/gnupg2/02-allow-mark-trusted.patch \
diff --git a/patches/gpgme-1.4.1/01-improve-trace.patch b/patches/gpgme-1.4.1/01-improve-trace.patch
new file mode 100755 (executable)
index 0000000..af8d55e
--- /dev/null
@@ -0,0 +1,92 @@
+#! /bin/sh
+patch -p1 -l -f $* < $0
+exit $?
+
+From 2118f497010a9a41c29d062a7605ff2e136f8f4e Mon Sep 17 00:00:00 2001
+From: Werner Koch <wk@gnupg.org>
+Date: Mon, 6 May 2013 20:23:47 +0200
+Subject: [PATCH 1/2] Improve debug output of the I/O reader and writer.
+
+* src/w32-io.c (reader, writer): Also print file_sock.
+---
+ src/w32-io.c |   22 +++++++++++++++++-----
+ 1 files changed, 17 insertions(+), 5 deletions(-)
+
+diff --git a/src/w32-io.c b/src/w32-io.c
+index cbc3064..164205e 100644
+--- a/src/w32-io.c
++++ b/src/w32-io.c
+@@ -264,8 +264,8 @@ reader (void *arg)
+   int nbytes;
+   DWORD nread;
+   int sock;
+-  TRACE_BEG1 (DEBUG_SYSIO, "gpgme:reader", ctx->file_hd,
+-            "thread=%p", ctx->thread_hd);
++  TRACE_BEG2 (DEBUG_SYSIO, "gpgme:reader", ctx->file_hd,
++            "file_sock=%d, thread=%p", ctx->file_sock, ctx->thread_hd);
+
+   if (ctx->file_hd != INVALID_HANDLE_VALUE)
+     sock = 0;
+@@ -400,6 +400,9 @@ create_reader (int fd)
+       TRACE_SYSERR (EIO);
+       return NULL;
+     }
++  TRACE_LOG4 ("fd=%d -> handle=%p socket=%d dupfrom=%d",
++              fd, fd_table[fd].handle, fd_table[fd].socket,
++              fd_table[fd].dup_from);
+   ctx->file_hd = fd_table[fd].handle;
+   ctx->file_sock = fd_table[fd].socket;
+
+@@ -652,8 +655,8 @@ writer (void *arg)
+   struct writer_context_s *ctx = arg;
+   DWORD nwritten;
+   int sock;
+-  TRACE_BEG1 (DEBUG_SYSIO, "gpgme:writer", ctx->file_hd,
+-            "thread=%p", ctx->thread_hd);
++  TRACE_BEG2 (DEBUG_SYSIO, "gpgme:writer", ctx->file_hd,
++            "file_sock=%d, thread=%p", ctx->file_sock, ctx->thread_hd);
+
+   if (ctx->file_hd != INVALID_HANDLE_VALUE)
+     sock = 0;
+@@ -766,6 +769,9 @@ create_writer (int fd)
+       TRACE_SYSERR (EIO);
+       return NULL;
+     }
++  TRACE_LOG4 ("fd=%d -> handle=%p socket=%d dupfrom=%d",
++              fd, fd_table[fd].handle, fd_table[fd].socket,
++              fd_table[fd].dup_from);
+   ctx->file_hd = fd_table[fd].handle;
+   ctx->file_sock = fd_table[fd].socket;
+
+@@ -1149,6 +1155,10 @@ _gpgme_io_close (int fd)
+       return TRACE_SYSRES (-1);
+     }
+
++  TRACE_LOG4 ("fd=%d -> handle=%p socket=%d dupfrom=%d",
++              fd, fd_table[fd].handle, fd_table[fd].socket,
++              fd_table[fd].dup_from);
++
+   kill_reader (fd);
+   kill_writer (fd);
+   LOCK (notify_table_lock);
+@@ -1544,7 +1554,7 @@ _gpgme_io_spawn (const char *path, char *const argv[], unsigned int flags,
+   args = calloc (2 + i + 1, sizeof (*args));
+   args[0] = (char *) _gpgme_get_w32spawn_path ();
+   args[1] = tmp_name;
+-  args[2] = path;
++  args[2] = (char *)path;
+   memcpy (&args[3], &argv[1], i * sizeof (*args));
+
+   memset (&sec_attr, 0, sizeof sec_attr);
+@@ -1734,7 +1744,9 @@ _gpgme_io_select (struct io_select_fd_s *fds, size_t nfds, int nonblock)
+   TRACE_BEG2 (DEBUG_SYSIO, "_gpgme_io_select", fds,
+               "nfds=%u, nonblock=%u", nfds, nonblock);
+
++#if 0
+  restart:
++#endif
+   TRACE_SEQ (dbg_help, "select on [ ");
+   any = 0;
+   nwait = 0;
+--
+1.7.7.1
diff --git a/patches/gpgme-1.4.1/02-close-socket.patch b/patches/gpgme-1.4.1/02-close-socket.patch
new file mode 100755 (executable)
index 0000000..b18f9c2
--- /dev/null
@@ -0,0 +1,78 @@
+#! /bin/sh
+patch -p1 -l -f $* < $0
+exit $?
+
+From 9f330be8210d2498fe93d4166b6f6c02fca76475 Mon Sep 17 00:00:00 2001
+From: Werner Koch <wk@gnupg.org>
+Date: Thu, 25 Apr 2013 12:00:16 +0100
+Subject: [PATCH 2/2] Fix hang in socket closing.
+
+* src/w32-io.c (destroy_reader): Call shutdown.
+(reader): Do not print an error in the shutdown case.
+---
+ src/w32-io.c |   36 ++++++++++++++++++++++++++++++++++++
+ 1 files changed, 36 insertions(+), 0 deletions(-)
+
+diff --git a/src/w32-io.c b/src/w32-io.c
+index 164205e..776e379 100644
+--- a/src/w32-io.c
++++ b/src/w32-io.c
+@@ -316,6 +316,21 @@ reader (void *arg)
+                 }
+               else
+                 {
++                  /* Check whether the shutdown triggered the error -
++                     no need to to print a warning in this case.  */
++                  if ( ctx->error_code == WSAECONNABORTED
++                       || ctx->error_code == WSAECONNRESET)
++                    {
++                      LOCK (ctx->mutex);
++                      if (ctx->stop_me)
++                        {
++                          UNLOCK (ctx->mutex);
++                          TRACE_LOG ("got shutdown");
++                          break;
++                        }
++                      UNLOCK (ctx->mutex);
++                    }
++
+                   ctx->error = 1;
+                   TRACE_LOG1 ("recv error: ec=%d", ctx->error_code);
+                 }
+@@ -357,6 +372,7 @@ reader (void *arg)
+          UNLOCK (ctx->mutex);
+          break;
+         }
++
+       TRACE_LOG1 ("got %u bytes", nread);
+
+       ctx->writepos = (ctx->writepos + nread) % READBUF_SIZE;
+@@ -495,6 +511,26 @@ destroy_reader (struct reader_context_s *ctx)
+     }
+ #endif
+
++  /* The reader thread is usually blocking in recv or ReadFile.  If
++     the peer does not send an EOF or breaks the pipe the WFSO might
++     get stuck waiting for the termination of the reader thread.  This
++     happens quite often with sockets, thus we definitely need to get
++     out of the recv.  A shutdown does this nicely.  For handles
++     (i.e. pipes) it would also be nice to cancel the operation, but
++     such a feature is only available since Vista.  Thus we need to
++     dlopen that syscall.  */
++  if (ctx->file_hd != INVALID_HANDLE_VALUE)
++    {
++      /* Fixme: Call CancelSynchronousIo (handle_of_thread).  */
++    }
++  else if (ctx->file_sock != INVALID_SOCKET)
++    {
++      if (shutdown (ctx->file_sock, 2))
++        TRACE2 (DEBUG_SYSIO, "gpgme:destroy_reader", ctx->file_hd,
++                "shutdown socket %d failed: %s",
++                ctx->file_sock, (int) WSAGetLastError ());
++    }
++
+   TRACE1 (DEBUG_SYSIO, "gpgme:destroy_reader", ctx->file_hd,
+           "waiting for termination of thread %p", ctx->thread_hd);
+   WaitForSingleObject (ctx->stopped, INFINITE);
+--
+1.7.7.1