[libvirt] [PATCH] command: avoid deadlock on EPIPE situation

[Michal Privoznik]

On 01.06.2012 17:41, Eric Blake wrote:
> It is possible to deadlock libvirt by having a domain with XML
> longer than PIPE_BUF, and by writing a hook script that closes
> stdin early.  This is because libvirt was keeping a copy of the
> child's stdin read fd open, which means the write fd in the
> parent will never see EPIPE (remember, libvirt should always be
> run with SIGPIPE ignored, so we should never get a SIGPIPE signal).
> Since there is no error, libvirt blocks waiting for a write to
> complete, even though the only reader is also libvirt.  The
> solution is to ensure that only the child can act as a reader
> before the parent does any writes; and then dealing with the
> fallout of dealing with EPIPE.
> 
> Thankfully, this is not a security hole - since the only way to
> trigger the deadlock is to install a custom hook script, anyone
> that already has privileges to install a hook script already has
> privileges to do any number of other equally disruptive things
> to libvirt; it would only be a security hole if an unprivileged
> user could install a hook script to DoS a privileged user.
> 
> * src/util/command.c (virCommandRun): Close parent's copy of child
> read fd earlier.
> (virCommandProcessIO): Don't let EPIPE be fatal; the child may
> be done parsing input.
> * tests/commandhelper.c (main): Set up a SIGPIPE situation.
> * tests/commandtest.c (test20): Trigger it.
> * tests/commanddata/test20.log: New file.
> ---
> 
> I tested this by using just the changes to 'tests/', and was able
> to trigger deadlock in 100% of my testing with the buffer lengths
> shown here.
> 
>  src/util/command.c           |   22 +++++++++++++---------
>  tests/commanddata/test20.log |   13 +++++++++++++
>  tests/commandhelper.c        |    6 ++++++
>  tests/commandtest.c          |   42 ++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 74 insertions(+), 9 deletions(-)
>  create mode 100644 tests/commanddata/test20.log

So we have now covered this case in tests. Awesome.

ACK

Michal