Richard W.M. Jones
2018-Nov-09 12:19 UTC
[Libguestfs] collectd leaks SIGCHLD == SIG_IGN into plugins
Peter Dimitrov and myself were debugging a very peculiar bug when libguestfs is run as a plugin from collectd: https://www.redhat.com/archives/libguestfs/2018-November/thread.html#00023 The long story short is that collectd leaks SIGCHLD == SIG_IGN setting into plugins: https://www.redhat.com/archives/libguestfs/2018-November/msg00095.html This means that any plugin that does the usual pattern of: pid = fork (); ... if (waitpid (pid, NULL, 0) == -1) { perror ("waitpid"); exit (EXIT_FAILURE); } will fail, because the forked subprocess is automatically reaped before waitpid is called, resulting in the wait failing with errno =ECHILD. It is possible to work around this by adding: signal (SIGCHLD, SIG_DFL); to the plugin. However I believe this is a bug in collectd, and it should sanitize signals (and maybe other things) before running plugins. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
Ruben Kerkhof
2018-Nov-13 08:19 UTC
Re: [Libguestfs] [collectd] collectd leaks SIGCHLD == SIG_IGN into plugins
Hi Rich, On Fri, Nov 9, 2018 at 1:19 PM, Richard W.M. Jones <rjones@redhat.com> wrote:> Peter Dimitrov and myself were debugging a very peculiar bug when > libguestfs is run as a plugin from collectd: > > https://www.redhat.com/archives/libguestfs/2018-November/thread.html#00023 > > The long story short is that collectd leaks SIGCHLD == SIG_IGN setting > into plugins: > > https://www.redhat.com/archives/libguestfs/2018-November/msg00095.html > > This means that any plugin that does the usual pattern of: > > pid = fork (); > ... > if (waitpid (pid, NULL, 0) == -1) { > perror ("waitpid"); > exit (EXIT_FAILURE); > } > > will fail, because the forked subprocess is automatically reaped > before waitpid is called, resulting in the wait failing with errno => ECHILD. > > It is possible to work around this by adding: > > signal (SIGCHLD, SIG_DFL); > > to the plugin. However I believe this is a bug in collectd, and it > should sanitize signals (and maybe other things) before running > plugins. > > Rich.Would you mind opening an issue at https://github.com/collectd/collectd/issues/new for this? Kind regards, Ruben Kerkhof
Richard W.M. Jones
2018-Nov-13 08:54 UTC
Re: [Libguestfs] [collectd] collectd leaks SIGCHLD == SIG_IGN into plugins
On Tue, Nov 13, 2018 at 09:19:40AM +0100, Ruben Kerkhof wrote:> Hi Rich, > > On Fri, Nov 9, 2018 at 1:19 PM, Richard W.M. Jones <rjones@redhat.com> wrote: > > Peter Dimitrov and myself were debugging a very peculiar bug when > > libguestfs is run as a plugin from collectd: > > > > https://www.redhat.com/archives/libguestfs/2018-November/thread.html#00023 > > > > The long story short is that collectd leaks SIGCHLD == SIG_IGN setting > > into plugins: > > > > https://www.redhat.com/archives/libguestfs/2018-November/msg00095.html > > > > This means that any plugin that does the usual pattern of: > > > > pid = fork (); > > ... > > if (waitpid (pid, NULL, 0) == -1) { > > perror ("waitpid"); > > exit (EXIT_FAILURE); > > } > > > > will fail, because the forked subprocess is automatically reaped > > before waitpid is called, resulting in the wait failing with errno => > ECHILD. > > > > It is possible to work around this by adding: > > > > signal (SIGCHLD, SIG_DFL); > > > > to the plugin. However I believe this is a bug in collectd, and it > > should sanitize signals (and maybe other things) before running > > plugins. > > > > Rich. > > Would you mind opening an issue at > https://github.com/collectd/collectd/issues/new for this?Peter: I don't have enough information (like collectd version etc) to fill in all the fields there. Since you are running collectd and encountered the problem originally, could you fill in the form requested above please, and then let me know the issue number so that I am able to follow it too. Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
Daniel P. Berrangé
2018-Nov-13 10:04 UTC
Re: [Libguestfs] collectd leaks SIGCHLD == SIG_IGN into plugins
On Fri, Nov 09, 2018 at 12:19:30PM +0000, Richard W.M. Jones wrote:> Peter Dimitrov and myself were debugging a very peculiar bug when > libguestfs is run as a plugin from collectd: > > https://www.redhat.com/archives/libguestfs/2018-November/thread.html#00023 > > The long story short is that collectd leaks SIGCHLD == SIG_IGN setting > into plugins: > > https://www.redhat.com/archives/libguestfs/2018-November/msg00095.html > > This means that any plugin that does the usual pattern of: > > pid = fork (); > ... > if (waitpid (pid, NULL, 0) == -1) { > perror ("waitpid"); > exit (EXIT_FAILURE); > } > > will fail, because the forked subprocess is automatically reaped > before waitpid is called, resulting in the wait failing with errno => ECHILD. > > It is possible to work around this by adding: > > signal (SIGCHLD, SIG_DFL); > > to the plugin. However I believe this is a bug in collectd, and it > should sanitize signals (and maybe other things) before running > plugins.Yes, I'm inclined to agree. If an app or library is spawning external processes it should take care to restore signal setup to a "normal" state. This means not only setting SIG_DFL for all signals, but also just as importantly, the signal mask. It should be set to all ones before calling fork, and set back to all zeroes immedaitely before execve, as illustrated in libvirt to avoid races: https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/util/vircommand.c;h=de937f6f9aa91abb518eac98bfac9dcf37e1f5df;hb=HEAD#l304 https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/util/vircommand.c;h=de937f6f9aa91abb518eac98bfac9dcf37e1f5df;hb=HEAD#l360 Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Richard W.M. Jones
2018-Nov-13 10:42 UTC
Re: [Libguestfs] collectd leaks SIGCHLD == SIG_IGN into plugins
On Tue, Nov 13, 2018 at 10:04:33AM +0000, Daniel P. Berrangé wrote:> On Fri, Nov 09, 2018 at 12:19:30PM +0000, Richard W.M. Jones wrote: > > Peter Dimitrov and myself were debugging a very peculiar bug when > > libguestfs is run as a plugin from collectd: > > > > https://www.redhat.com/archives/libguestfs/2018-November/thread.html#00023 > > > > The long story short is that collectd leaks SIGCHLD == SIG_IGN setting > > into plugins: > > > > https://www.redhat.com/archives/libguestfs/2018-November/msg00095.html > > > > This means that any plugin that does the usual pattern of: > > > > pid = fork (); > > ... > > if (waitpid (pid, NULL, 0) == -1) { > > perror ("waitpid"); > > exit (EXIT_FAILURE); > > } > > > > will fail, because the forked subprocess is automatically reaped > > before waitpid is called, resulting in the wait failing with errno => > ECHILD. > > > > It is possible to work around this by adding: > > > > signal (SIGCHLD, SIG_DFL); > > > > to the plugin. However I believe this is a bug in collectd, and it > > should sanitize signals (and maybe other things) before running > > plugins. > > Yes, I'm inclined to agree. If an app or library is spawning external > processes it should take care to restore signal setup to a "normal" state. > This means not only setting SIG_DFL for all signals, but also just as > importantly, the signal mask. It should be set to all ones before calling > fork, and set back to all zeroes immedaitely before execve, as illustrated > in libvirt to avoid races: > > https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/util/vircommand.c;h=de937f6f9aa91abb518eac98bfac9dcf37e1f5df;hb=HEAD#l304 > > https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/util/vircommand.c;h=de937f6f9aa91abb518eac98bfac9dcf37e1f5df;hb=HEAD#l360This is absolutely right. What's interesting and new here is that collectd is actually using dlopen to call external plugins (so not forking itself). See collectd.git/src/daemon/plugin.c However it's still not sanitizing the environment sufficiently around these calls, so it's the same sort of problem, but in a different context that I'd not seen/understood before. nbdkit has the same problem :-( We already dealt with umask a few years ago, now I've got to check our signal handlers too ... Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
Possibly Parallel Threads
- Re: collectd leaks SIGCHLD == SIG_IGN into plugins
- collectd leaks SIGCHLD == SIG_IGN into plugins
- Re: [collectd] collectd leaks SIGCHLD == SIG_IGN into plugins
- Re: [collectd] collectd leaks SIGCHLD == SIG_IGN into plugins
- Re: [collectd] collectd leaks SIGCHLD == SIG_IGN into plugins