[libvirt] [PATCH] Ignore SIGWINCH in remote client call to poll(2) (RHBZ#567931).

[Richard W.M. Jones]

On Wed, Feb 24, 2010 at 03:51:39PM +0000, Daniel P. Berrange wrote:
> I think SIGWINCH + SIGCHLD + SIGPIPE are a reasonably set to block
> since those are common "normal" signals a process may received during
> operation. Other signals are all typically related to fatal problems
> rather tha normal conditions

This updated patch includes masking of SIGCHLD and SIGPIPE as well.  I
tested this patch with virt-top.

Another potential change would be to use ppoll(2) instead of poll(2).
However I don't think this gives us any extra safety.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://et.redhat.com/~rjones/libguestfs/
See what it can do: http://et.redhat.com/~rjones/libguestfs/recipes.html
-------------- next part --------------
>From f4a43df52b7c84bda61863250d20135f044893da Mon Sep 17 00:00:00 2001
From: Richard Jones <rjones at redhat.com>
Date: Wed, 24 Feb 2010 12:55:17 +0000
Subject: [PATCH] Ignore SIGWINCH in remote client call to poll(2) (RHBZ#567931).

In bug 567931 we found that virt-top would exit occasionally
when the terminal window was resized.  Tracking this down it
turned out that SIGWINCH was being delivered to the process at
exactly the point where the libvirt remote driver was calling
poll(2) waiting for a reply from libvirtd.

This caused the poll(2) call to be interrupted (returning errno
EINTR).  However handling EINTR the same way as EAGAIN was not
the solution to this problem since we found previously that this
would break Ctrl-C handling (commit 47fec8eac2bb3).

The correct solution is to mask out SIGWINCH for the duration
of the poll(2) system call.  The per-thread mask is changed and
restored immediately after the call.  Since we are using
pthread_sigmask, this should not affect other threads, and
since we restore the signal mask immediately afterwards it should
not affect the current thread visibly either.  Other possibly
problematic signals are SIGCHLD and SIGPIPE and these are
masked too.

Note use of ignore_value: It's not fatal if we cannot mask out
SIGWINCH, and in any case pthread_sigmask never fails on Linux
as long as you supply the correct arguments.

I tested this patch and it cures the original problem with
virt-top.
---
 src/remote/remote_driver.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 8914c39..2060cf6 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -87,6 +87,7 @@
 #include "memory.h"
 #include "util.h"
 #include "event.h"
+#include "ignore-value.h"
 
 #define VIR_FROM_THIS VIR_FROM_REMOTE
 
@@ -8386,6 +8387,7 @@ remoteIOEventLoop(virConnectPtr conn,
         struct remote_thread_call *tmp = priv->waitDispatch;
         struct remote_thread_call *prev;
         char ignore;
+        sigset_t oldmask, blockedsigs;
 
         fds[0].events = fds[0].revents = 0;
         fds[1].events = fds[1].revents = 0;
@@ -8407,10 +8409,24 @@ remoteIOEventLoop(virConnectPtr conn,
          * can stuff themselves on the queue */
         remoteDriverUnlock(priv);
 
+        /* Block SIGWINCH from interrupting poll in curses programs,
+         * then restore the original signal mask again immediately
+         * after the call (RHBZ#567931).  Same for SIGCHLD and SIGPIPE
+         * at the suggestion of Paolo Bonzini and Daniel Berrange.
+         */
+        sigemptyset (&blockedsigs);
+        sigaddset (&blockedsigs, SIGWINCH);
+        sigaddset (&blockedsigs, SIGCHLD);
+        sigaddset (&blockedsigs, SIGPIPE);
+        ignore_value (pthread_sigmask(SIG_BLOCK, &blockedsigs, &oldmask));
+
     repoll:
         ret = poll(fds, ARRAY_CARDINALITY(fds), -1);
         if (ret < 0 && errno == EAGAIN)
             goto repoll;
+
+        ignore_value (pthread_sigmask(SIG_SETMASK, &oldmask, NULL));
+
         remoteDriverLock(priv);
 
         if (fds[1].revents) {
-- 
1.6.5.2