[libvirt] [PATCH v2 4/4] virCommand: use procfs to learn opened FDs

[Ján Tomko]

On Tue, Jul 16, 2019 at 10:54:36AM +0200, Michal Privoznik wrote:
>When spawning a child process, between fork() and exec() we close
>all file descriptors and keep only those the caller wants us to
>pass onto the child. The problem is how we do that. Currently, we
>get the limit of opened files and then iterate through each one
>of them and either close() it or make it survive exec(). This
>approach is suboptimal (although, not that much in default
>configurations where the limit is pretty low - 1024). We have
>/proc where we can learn what FDs we hold open and thus we can
>selectively close only those.
>
>Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>---
> src/util/vircommand.c | 86 +++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 78 insertions(+), 8 deletions(-)
>
>diff --git a/src/util/vircommand.c b/src/util/vircommand.c
>index 6cd7cbe065..bfc6c15cfb 100644
>--- a/src/util/vircommand.c
>+++ b/src/util/vircommand.c
>@@ -418,27 +418,97 @@ virExecCommon(virCommandPtr cmd, gid_t *groups, int ngroups)
> static int
> virCommandMassClose(virCommandPtr cmd,
>                     int childin,
>                     int childout,
>                     int childerr)
> {
>+    VIR_AUTOPTR(virBitmap) fds = NULL;
>     int openmax = sysconf(_SC_OPEN_MAX);
>-    int fd;
>-    int tmpfd;
>+    int fd = -1;
>
>-    if (openmax < 0) {
>-        virReportSystemError(errno,  "%s",
>-                             _("sysconf(_SC_OPEN_MAX) failed"));
>+    /* In general, it is not save to call malloc() between fork() and exec()

s/save/safe/

>+     * because the child might have forked at the worst possible time, i.e.
>+     * when another thread was in malloc() and thus held its lock. That is to
>+     * say, POSIX does not mandate malloc() to be async-safe. Fortunately,
>+     * glibc developers are aware of this and made malloc() async-safe.
>+     * Therefore we can safely allocate memory here (and transitively call
>+     * opendir/readdir) without a deadlock. */
>+
>+    if (!(fds = virBitmapNew(openmax)))
>+        return -1;
>+
>+# ifdef __linux__
>+    if (virCommandMassCloseGetFDsLinux(cmd, fds) < 0)
>+        return -1;
>+# else
>+    if (virCommandMassCloseGetFDsGeneric(cmd, fds) < 0)
>         return -1;
>-    }
>+# endif
>
>-    for (fd = 3; fd < openmax; fd++) {
>+    fd = virBitmapNextSetBit(fds, -1);
>+    for (; fd >= 0; fd = virBitmapNextSetBit(fds, fd)) {

fd >= 3 to make it match the previous behavior

>         if (fd == childin || fd == childout || fd == childerr)
>             continue;
>         if (!virCommandFDIsSet(cmd, fd)) {
>-            tmpfd = fd;
>+            int tmpfd = fd;
>             VIR_MASS_CLOSE(tmpfd);

Reviewed-by: Ján Tomko <jtomko at redhat.com>

Jano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190716/944adb5c/attachment-0001.sig>