Hi Rich, On Fri, Nov 9, 2018 at 1:19 PM, Richard W.M. Jones <rjones(a)redhat.com&gt; 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?

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=d... https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/util/vircommand.c;h=d...

> > This means that any plugin that does the usual pattern of: > > > > pid = fork ();

A quick glance shows that the exec plugin actually is clearing the signal mask: https://github.com/collectd/collectd/blob/master/src/exec.c#L526

Can you give some more context when this problem comes up? Ideally in a Github issue (hint, hint ;).

On Tue, Nov 13, 2018 at 12:00:19PM +0100, Florian Forster wrote: > Hi, > > thank you very much for reporting this! Sounds like a bug in the exec plugin – > it never ceases to amaze me how many issues a single plugin can have ;) > > > > > This means that any plugin that does the usual pattern of: > > > > > > > > pid = fork (); > > Note that the exec plugin is the *only* plugin that does this. All other > plugins are forbidden to fork(), popen() or create new processes in any other > way. The only plugin doing that, the exec plugin, has had enough issues over > the years for me to feel justified in that decision. ;-) > > As mentioned before, a Github issue would be appreciated so we can properly > track this problem. I don't know if Peter is using the exec plugin or is trying to write an ordinary plugin. However the library he is using (libguestfs) certainly does fork subprocess(es). Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html

On Tue, Nov 13, 2018 at 03:17:33PM +0200, Peter Dimitrov wrote: > I was extending the virt plugin. > It already collects similar data (about VMs) using Libvirt's API, but lacks > disk usage information. > > I went through some hoops to link libguestfs correctly to collectd. > Is it okay to just include fork(), waitpid() example? It does reproduce the > issue. Yes, I think a small example/reproducer is a fine idea. Rich. > Best Regards, > Peter > > > On Tue, Nov 13, 2018 at 1:34 PM Richard W.M. Jones <rjones(a)redhat.com&gt; > wrote: > > > On Tue, Nov 13, 2018 at 12:00:19PM +0100, Florian Forster wrote: > > > Hi, > > > > > > thank you very much for reporting this! Sounds like a bug in the exec > > plugin – > > > it never ceases to amaze me how many issues a single plugin can have ;) > > > > > > > > > This means that any plugin that does the usual pattern of: > > > > > > > > > > > > pid = fork (); > > > > > > Note that the exec plugin is the *only* plugin that does this. All other > > > plugins are forbidden to fork(), popen() or create new processes in any > > other > > > way. The only plugin doing that, the exec plugin, has had enough issues > > over > > > the years for me to feel justified in that decision. ;-) > > > > > > As mentioned before, a Github issue would be appreciated so we can > > properly > > > track this problem. > > > > I don't know if Peter is using the exec plugin or is trying to write > > an ordinary plugin. However the library he is using (libguestfs) > > certainly does fork subprocess(es). > > > > Rich. > > > > -- > > Richard Jones, Virtualization Group, Red Hat > > http://people.redhat.com/~rjones > > Read my programming and virtualization blog: http://rwmj.wordpress.com > > virt-builder quickly builds VMs from scratch > > http://libguestfs.org/virt-builder.1.html > > -- 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