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 to clean up fds on fork so it is immune to >> anything we might leak; for libnbd because we don't use the API that >> forks). Note that I did not choose to set the new field for many of >> the various language bindings (it becomes a rather difficult task to >> prove whether the third-party language binding code is itself using >> atomic CLOEXEC or fd sanitization). However many of our languages are >> still stuck as serialized, and the lack of .fork_safe won't impact >> those thread model anyways. >> >> Update the testsuite to skip parallel tests that would otherwise fail >> when the thread model is crippled. >> >> Upcoming patches will then fix the server to audit and fix places >> where we currently leak fds, and then cripple the thread model only on >> platforms where atomic CLOEXEC is not possible. > > My worry about this patch is we're adding a new plugin flag which > we'll have to support forever, but IIUC it's only needed on one > platform (ie. Haiku) which really ought to get fixed. In future we'll > end up in a situation where we have this flag but it's no longer > needed.
Yeah, the fact that it is a no-op on Linux means it's hard to test. Still, the idea of penalizing all plugins, even though only the forking plugins care, is a bit hard to swallow - maybe that will prod Haiku in catching up faster. > How about instead of this we simply restrict Haiku to the fully > serialized mode. Sucks for them, but they can fix it by adding atomic > CLOEXEC features ... Makes sense. I'll still want to ensure that --dump-plugin shows the dynamic crippling (so that we can easily skip tests/test-parallel-file.sh, for example), but I think I can manage to do that by replacing this patch with just blind penalization of non-atomic CLOEXEC. > >> + .fork_safe = 0, /* libguestfs uses fork(), unaudited for safety */ > > libguestfs does use fork and should be safe - we're pretty careful > about using CLOEXEC, accept4, etc everywhere. (Also libguestfs > doesn't run on Haiku and architecturally that's unlikely to ever > happen). Nice to know. That was half of the review - determining which libraries use which functions, and what might still need fixing (or bugs filed to other projects). It also looks like libnbd is using CLOEXEC everywhere that it should. But as long as the fault is no longer nbdkit's, any further bugs we get about fd leaks can be redirected to whoever is actually doing the leak. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Libguestfs mailing list [email protected] https://www.redhat.com/mailman/listinfo/libguestfs
