search for: cloexec

On Sat, Aug 06, 2016 at 03:43:48AM +0000, Eric Wong wrote: > Eric Wong <e at 80x24.org> wrote: > > I'm trying to test a trivial patch to set FD_CLOEXEC on the > > flintlock lockfd when using F_OFD_SETLK > > Fwiw, this is the patch I was originally going to test. > (but now I see maybe my F_SETFD might only need to be > called on F_OFD_SETLK success) Description at bottom. > > --- a/backends/flint_lock.cc > +++ b/bac...

On 7/31/19 4:31 PM, Eric Blake wrote: > The rate filter is potentially opening fds in one thread while another > thread is processing a fork() in the plugin. Although the file is not > open for long, we MUST atomically use CLOEXEC to avoid fd leaks. This > one is a bit harder to observe using only the sh plugin, because the > window is small; you'll have better success at catching the leak by > using gdb or recompiling code to insert strategic sleeps. In fact, I have to tweak this commit message: you CAN'T...

On Fri, Aug 02, 2019 at 02:26:11PM -0500, Eric Blake wrote: > Allow a plugin field to declare whether a parallel plugin can tolerate > windows where fds are not CLOEXEC, or must take precautions to avoid > leaking fds if the plugin may fork. For safety reasons, the flag > defaults to off, but many in-tree plugins can set it to on (most > commonly because they don't fork after .config_complete; for libvirt > because it is documented to clean up fds...

Technically, as long as our thread model is SERIALIZE_ALL_REQUESTS, we don't have to be very careful about atomic CLOEXEC on any of the pipes we create for communication with the child. However, the next patch wants to promote sh plugin to parallel when possible, which requires the use of pipe2 to avoid fd leaks. Also, add an assert to ensure that we avoid dup2(n, n) (which would fail to clear CLOEXEC) and close(0-2...

...:01:52PM -0500, Eric Blake wrote: >> On 7/31/19 4:31 PM, Eric Blake wrote: >>> The rate filter is potentially opening fds in one thread while another >>> thread is processing a fork() in the plugin. Although the file is not >>> open for long, we MUST atomically use CLOEXEC to avoid fd leaks. This >>> one is a bit harder to observe using only the sh plugin, because the >>> window is small; you'll have better success at catching the leak by >>> using gdb or recompiling code to insert strategic sleeps. >> >> In fact, I have to...

...1, 2019 at 05:01:52PM -0500, Eric Blake wrote: > On 7/31/19 4:31 PM, Eric Blake wrote: > > The rate filter is potentially opening fds in one thread while another > > thread is processing a fork() in the plugin. Although the file is not > > open for long, we MUST atomically use CLOEXEC to avoid fd leaks. This > > one is a bit harder to observe using only the sh plugin, because the > > window is small; you'll have better success at catching the leak by > > using gdb or recompiling code to insert strategic sleeps. > > In fact, I have to tweak this comm...

On platforms that lack atomic CLOEXEC, it's simpler to just always force serialization (no client thread will be executing when nbdkit itself is creating a new file descriptor) than to audit which plugins actually care about not getting any leaked fds. We have one final function that we need to use for atomic CLOEXEC; the next pat...

I'm trying to test a trivial patch to set FD_CLOEXEC on the flintlock lockfd when using F_OFD_SETLK and am running into a build failure even in an unpatched state. This is on a Debian jessie amd64 system building the jessie-backports version. The stable version (1.2.19-1+deb8u1) works and builds fine using the same commands, however I also encounte...

...00644 --- a/plugins/sh/call.c +++ b/plugins/sh/call.c @@ -94,6 +94,10 @@ call3 (const char *wbuf, size_t wbuflen, /* sent to stdin */ *rbuflen = *ebuflen = 0; rbufalloc = ebufalloc = 0; + /* As long as we use NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS, we + * don't have to worry about CLOEXEC, because we know no other + * thread is competing to fork at the same time as this one. + */ if (pipe (in_fd) == -1) { nbdkit_error ("%s: pipe: %m", script); goto error; diff --git a/plugins/sh/sh.c b/plugins/sh/sh.c index aeb01918..1e000b11 100644 --- a/plugins/sh/sh.c +...

This patch series is fine, ACK. Unfortunately it likely breaks Haiku support. I'd like to hear from the Haiku folk if they are planning support for atomic CLOEXEC (SOCK_CLOEXEC, accept4, pipe2, etc.) 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 8/2/19 2:52 PM, Richard W.M. Jones wrote: > On Fri, Aug 02, 2019 at 02:26:11PM -0500, Eric Blake wrote: >> Allow a plugin field to declare whether a parallel plugin can tolerate >> windows where fds are not CLOEXEC, or must take precautions to avoid >> leaking fds if the plugin may fork. For safety reasons, the flag >> defaults to off, but many in-tree plugins can set it to on (most >> commonly because they don't fork after .config_complete; for libvirt >> because it is documented...

The rate filter is potentially opening fds in one thread while another thread is processing a fork() in the plugin. Although the file is not open for long, we MUST atomically use CLOEXEC to avoid fd leaks. This one is a bit harder to observe using only the sh plugin, because the window is small; you'll have better success at catching the leak by using gdb or recompiling code to insert strategic sleeps. Although fopen("re") is not required by POSIX yet, it has been d...

...it all existing users of pipe, to see if they should be using pipe2. The tests fork() but don't fail because of fd leaks; and the nbd plugin doesn't fork() but was merely using pipe2 for convenience over multiple fcntl calls. However, the server's quit_fd definitely needs to be marked CLOEXEC (it's easy to use the sh plugin to show that we are currently leaking it to children), although doing so can occur without worrying about atomicity since it is called before threads begin. Finally, it's also worth updating our set_cloexec helper function to document that we still prefer at...

...1 are closed when we start pipe()ing, so our first pipe will claim 0 and 1 in either order, then the first check will either be a no-op (because fd[0] is 0 and doesn't need moving) or else fd[0] will be 1 and needs to overwrite stdin. Overwriting is fine; but if fd[0] is 0, we HAVE to clear FD_CLOEXEC, otherwise stdin will be closed by exec(). But it's close; so I'll use it as the starting point and push the corrected version soon. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org

...fully parallel, on capable platforms. See also my question on patch 10 on whether I've picked the best naming convention. Eric Blake (17): maint: Rename server/utils.c to server/public.c plugins: Expose more thread_model details in --dump-plugin server: Add utility functions for setting CLOEXEC and NONBLOCK Revert "RHEL 5: Define O_CLOEXEC and SOCK_CLOEXEC." build: Audit existing use of SOCK_CLOEXEC cow, cache: Better mkostemp fallback build: Audit for use of pipe2 rate: Atomically set CLOEXEC on fds server: Use atomic CLOEXEC for nbdkit_read_password plugins: Add...

On 8/27/19 7:55 AM, Eric Blake wrote: > On 8/27/19 6:47 AM, Richard W.M. Jones wrote: >> On Fri, Aug 02, 2019 at 02:26:15PM -0500, Eric Blake wrote: >>> + /* Ensure that stdin/out/err of the current process were not empty >>> + * before we started creating pipes (otherwise, the close and dup2 >>> + * calls below become more complex to juggle fds around

There's enough here to need a review; some of it probably needs backporting to stable-1.12. This probably breaks tests on Haiku or other platforms that have not been as on-the-ball about atomic CLOEXEC; feel free to report issues that arise, and I'll help come up with workarounds (even if we end up leaving a rare fd leak on less-capable systems). Meanwhile, I'm still working on my promise to add an nbdkit_nanosleep for use in the delay and stat filters, and which makes nbdkit more respon...

...temp (h->sa_tmpdir) == NULL) { > + SET_NEXT_STATE (%.DEAD); > + set_error (errno, "mkdtemp"); > + return 0; > + } > + len = strlen (h->sa_tmpdir); > + memcpy (h->sa_sockpath, h->sa_tmpdir, len); > + > + s = socket (AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC, 0); > + if (s == -1) { > + SET_NEXT_STATE (%.DEAD); > + set_error (errno, "socket"); > + return 0; > + } If we fail here or later, do/should we try to clean up the /tmp/libnbdXXX directory created earlier? /me reads ahead - nbd_close tries to address it Stil...

On Fri, Aug 02, 2019 at 02:26:15PM -0500, Eric Blake wrote: > + /* Ensure that stdin/out/err of the current process were not empty > + * before we started creating pipes (otherwise, the close and dup2 > + * calls below become more complex to juggle fds around correctly). > + */ > + assert (in_fd[0] > STDERR_FILENO && in_fd[1] > STDERR_FILENO && > +

...n to the fd of the suspend-image, which blocked the umount. Our theory is that when libxl forks and executes the qemu process, qemu inherits all currentl open fds in the xenopsd process, and never gives them away anymore. We could solve this by opening the suspend-image file in xenopsd using the O_CLOEXEC flag, causing the file to be closed when executing qemu. However, we are worried that this behaviour may still cause trouble in multi-threaded programs such as xenopsd, unless all files it ever opens are opened with O_CLOEXEC, which is easily forgotten. Would it be possible for libxl to spawn qemu...