[PATCH 0/2] tests: Stabilize tests when environment leaks FDs into them
Jirka reported to me that one of the test cases I've introduced started to fail for him. It turned out that flatpak "leaks" a FD of the wayland socket to run programs in his env, which broke a test case wanting to use fd '205'. (we use stable FD numbers via dup2 to ensure output stays identical) To prevent this from happening refactor virCommandMassClose and use it in all tests to close everything except stdio. Peter Krempa (2): util: command: Extract non-virCommand related bits from virCommandMassClose testutils: Close FDs leaked into test programs to avoid random breakage src/libvirt_private.syms | 1 + src/util/vircommand.c | 101 +++++++++++++++++++++------------------ src/util/vircommand.h | 3 ++ tests/testutils.c | 10 ++++ 4 files changed, 69 insertions(+), 46 deletions(-) -- 2.54.0
From: Peter Krempa <pkrempa@redhat.com> Extract the common code used to pass FDs from a virCommand to a child process into virCommandMassClose - a new wrapper - and create 'virCommandFDMassClose' which simply takes a bitmap of FDs to be kept and closes everything else. This will allow reuse of the algorithm in test files where we want to prevent FDs leaked from the environment from breakign the test. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/vircommand.c | 101 +++++++++++++++++++++------------------ src/util/vircommand.h | 3 ++ 3 files changed, 59 insertions(+), 46 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f22b5895db..744932acd8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2182,6 +2182,7 @@ virCommandDoAsyncIO; virCommandDryRunTokenFree; virCommandDryRunTokenNew; virCommandExec; +virCommandFDMassClose; virCommandFree; virCommandGetArgList; virCommandGetBinaryPath; diff --git a/src/util/vircommand.c b/src/util/vircommand.c index e871d572a6..07e7040ef8 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -520,16 +520,12 @@ virCommandMassCloseGetFDs(virBitmap *fds) } static int -virCommandMassCloseFrom(virCommand *cmd, - int childin, - int childout, - int childerr) +virCommandMassCloseFrom(virBitmap *keep) { g_autoptr(virBitmap) fds = NULL; int openmax = sysconf(_SC_OPEN_MAX); - int lastfd = -1; + int lastfd = virBitmapLastSetBit(keep); int fd = -1; - size_t i; /* In general, it is not safe to call malloc() between fork() and exec() * because the child might have forked at the worst possible time, i.e. @@ -551,24 +547,14 @@ virCommandMassCloseFrom(virCommand *cmd, if (virCommandMassCloseGetFDs(fds) < 0) return -1; - lastfd = MAX(lastfd, childin); - lastfd = MAX(lastfd, childout); - lastfd = MAX(lastfd, childerr); - - for (i = 0; i < cmd->npassfd; i++) - lastfd = MAX(lastfd, cmd->passfd[i].fd); - fd = virBitmapNextSetBit(fds, 2); for (; fd >= 0 && fd <= lastfd; fd = virBitmapNextSetBit(fds, fd)) { - if (fd == childin || fd == childout || fd == childerr) + int tmpfd = fd; + + if (virBitmapIsBitSet(keep, fd)) continue; - if (!virCommandFDIsSet(cmd, fd)) { - int tmpfd = fd; - VIR_MASS_CLOSE(tmpfd); - } else if (virSetInherit(fd, true) < 0) { - virReportSystemError(errno, _("failed to preserve fd %1$d"), fd); - return -1; - } + + VIR_MASS_CLOSE(tmpfd); } if (virCloseFrom(lastfd + 1) < 0) { @@ -588,33 +574,13 @@ virCommandMassCloseFrom(virCommand *cmd, static int -virCommandMassCloseRange(virCommand *cmd, - int childin, - int childout, - int childerr) +virCommandMassCloseRange(virBitmap *keep) { - g_autoptr(virBitmap) fds = virBitmapNew(0); ssize_t first; ssize_t last; - size_t i; - - virBitmapSetBitExpand(fds, childin); - virBitmapSetBitExpand(fds, childout); - virBitmapSetBitExpand(fds, childerr); - - for (i = 0; i < cmd->npassfd; i++) { - int fd = cmd->passfd[i].fd; - - virBitmapSetBitExpand(fds, fd); - - if (virSetInherit(fd, true) < 0) { - virReportSystemError(errno, _("failed to preserve fd %1$d"), fd); - return -1; - } - } first = 2; - while ((last = virBitmapNextSetBit(fds, first)) >= 0) { + while ((last = virBitmapNextSetBit(keep, first)) >= 0) { if (first + 1 == last) { first = last; continue; @@ -642,17 +608,60 @@ virCommandMassCloseRange(virCommand *cmd, } +/** + * virCommandFDMassClose: + * @keep: bitmap of FDs to be kept open in the child process + * + * Closes all open FDs (in the current process) except those represented by + * set bits in @keep. + * + * Returns 0 on success, -1 on error and reports libvirt errors. + */ +int +virCommandFDMassClose(virBitmap *keep) +{ + if (virCloseRangeIsSupported()) + return virCommandMassCloseRange(keep); + + return virCommandMassCloseFrom(keep); +} + +/** + * virCommandMassClose: + * @cmd: command structure + * @childin: fd passed to child as stdin + * @childout: fd passed to child as stdout + * @childerr: fd passed to child as stderr + * + * Prepares FDs to be passed to the child process spawned by @cmd and closes + * every other FD. + */ static int virCommandMassClose(virCommand *cmd, int childin, int childout, int childerr) { - if (virCloseRangeIsSupported()) - return virCommandMassCloseRange(cmd, childin, childout, childerr); + g_autoptr(virBitmap) fds = virBitmapNew(0); + size_t i; + + virBitmapSetBitExpand(fds, childin); + virBitmapSetBitExpand(fds, childout); + virBitmapSetBitExpand(fds, childerr); + + for (i = 0; i < cmd->npassfd; i++) { + int fd = cmd->passfd[i].fd; + + virBitmapSetBitExpand(fds, fd); + + if (virSetInherit(fd, true) < 0) { + virReportSystemError(errno, _("failed to preserve fd %1$d"), fd); + return -1; + } + } - return virCommandMassCloseFrom(cmd, childin, childout, childerr); + return virCommandFDMassClose(fds); } diff --git a/src/util/vircommand.h b/src/util/vircommand.h index a6e9a9a165..523ad1297d 100644 --- a/src/util/vircommand.h +++ b/src/util/vircommand.h @@ -23,6 +23,7 @@ #include "internal.h" #include "virbuffer.h" +#include "virbitmap.h" typedef struct _virCommandSendBuffer virCommandSendBuffer; struct _virCommandSendBuffer { @@ -32,6 +33,8 @@ struct _virCommandSendBuffer { off_t offset; }; +int virCommandFDMassClose(virBitmap *keep); + typedef struct _virCommand virCommand; /* This will execute in the context of the first child -- 2.54.0
On Wed, May 13, 2026 at 12:14:55 +0200, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
Extract the common code used to pass FDs from a virCommand to a child process into virCommandMassClose - a new wrapper - and create 'virCommandFDMassClose' which simply takes a bitmap of FDs to be kept and closes everything else.
This will allow reuse of the algorithm in test files where we want to prevent FDs leaked from the environment from breakign the test.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/vircommand.c | 101 +++++++++++++++++++++------------------ src/util/vircommand.h | 3 ++ 3 files changed, 59 insertions(+), 46 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f22b5895db..744932acd8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2182,6 +2182,7 @@ virCommandDoAsyncIO; virCommandDryRunTokenFree; virCommandDryRunTokenNew; virCommandExec; +virCommandFDMassClose;
[1]
virCommandFree; virCommandGetArgList; virCommandGetBinaryPath; diff --git a/src/util/vircommand.c b/src/util/vircommand.c index e871d572a6..07e7040ef8 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c +/** + * virCommandFDMassClose: + * @keep: bitmap of FDs to be kept open in the child process + * + * Closes all open FDs (in the current process) except those represented by + * set bits in @keep. + * + * Returns 0 on success, -1 on error and reports libvirt errors. + */ +int +virCommandFDMassClose(virBitmap *keep) +{ + if (virCloseRangeIsSupported()) + return virCommandMassCloseRange(keep); + + return virCommandMassCloseFrom(keep); +}
I didn't notice that most of this file is in a #ifndef WIN32 block, so compiler on mingw isn't happy with [1]. I'll need to add an override.
From: Peter Krempa <pkrempa@redhat.com> Some test cases (such as cases excercising FD passing in qemuxmlconftest) use dup2 to put FDs on certain numbers to have stable test output. If the environment the test program is run in "leaks"/passes some FDs the test may file in a very obscure way. Since the tests are designed to be standalone we can simply close everything except std(in|out|err) before running the tests to avoid unstable tests. Use virCommandFDMassClose to close everything except stdio. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/testutils.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/testutils.c b/tests/testutils.c index 35571fb2ad..9ce549370b 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -33,6 +33,7 @@ #include "virbuffer.h" #include "virlog.h" #include "virstring.h" +#include "vircommand.h" #define VIR_FROM_THIS VIR_FROM_NONE @@ -840,6 +841,7 @@ int virTestMain(int argc, int (*func)(void), ...) { + g_autoptr(virBitmap) keepfds = virBitmapNew(0); const char *lib; va_list ap; int ret; @@ -859,6 +861,14 @@ int virTestMain(int argc, preloads[npreloads] = NULL; } + /* Environment may leak FDs into the test which we don't want to use and + * it could break certain test cases which use 'dup2' to put FDs to correct + * numbers to have stable output. */ + virBitmapSetBitExpand(keepfds, STDIN_FILENO); + virBitmapSetBitExpand(keepfds, STDOUT_FILENO); + virBitmapSetBitExpand(keepfds, STDERR_FILENO); + virCommandFDMassClose(keepfds); + va_start(ap, func); while ((lib = va_arg(ap, const char *))) { g_autofree char *abs_lib_path = g_strdup_printf("%s/%s", abs_builddir, lib); -- 2.54.0
On Wed, May 13, 2026 at 12:14:56PM +0200, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
Some test cases (such as cases excercising FD passing in qemuxmlconftest) use dup2 to put FDs on certain numbers to have stable test output. If the environment the test program is run in "leaks"/passes some FDs the test may file in a very obscure way.
Since the tests are designed to be standalone we can simply close everything except std(in|out|err) before running the tests to avoid unstable tests.
Use virCommandFDMassClose to close everything except stdio.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/testutils.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/tests/testutils.c b/tests/testutils.c index 35571fb2ad..9ce549370b 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -33,6 +33,7 @@ #include "virbuffer.h" #include "virlog.h" #include "virstring.h" +#include "vircommand.h"
#define VIR_FROM_THIS VIR_FROM_NONE
@@ -840,6 +841,7 @@ int virTestMain(int argc, int (*func)(void), ...) { + g_autoptr(virBitmap) keepfds = virBitmapNew(0); const char *lib; va_list ap; int ret; @@ -859,6 +861,14 @@ int virTestMain(int argc, preloads[npreloads] = NULL; }
+ /* Environment may leak FDs into the test which we don't want to use and + * it could break certain test cases which use 'dup2' to put FDs to correct + * numbers to have stable output. */ + virBitmapSetBitExpand(keepfds, STDIN_FILENO); + virBitmapSetBitExpand(keepfds, STDOUT_FILENO); + virBitmapSetBitExpand(keepfds, STDERR_FILENO); + virCommandFDMassClose(keepfds);
I'm not convinced this is safe to do if it is not followed by an immediate exec. Libraries we link to are liable to open file descriptors, either in constructors, or in Libc code that runs before main(). By doing a mass close we are at risk of library code using a bad FD. While the tests may pass CI with this today, it feels like a foot-gun waiting to go off.
+ va_start(ap, func); while ((lib = va_arg(ap, const char *))) { g_autofree char *abs_lib_path = g_strdup_printf("%s/%s", abs_builddir, lib); -- 2.54.0
With regards, Daniel -- |: https://berrange.com ~~ https://hachyderm.io/@berrange :| |: https://libvirt.org ~~ https://entangle-photo.org :| |: https://pixelfed.art/berrange ~~ https://fstop138.berrange.com :|
On Wed, May 13, 2026 at 11:23:18 +0100, Daniel P. Berrangé wrote:
On Wed, May 13, 2026 at 12:14:56PM +0200, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
Some test cases (such as cases excercising FD passing in qemuxmlconftest) use dup2 to put FDs on certain numbers to have stable test output. If the environment the test program is run in "leaks"/passes some FDs the test may file in a very obscure way.
Since the tests are designed to be standalone we can simply close everything except std(in|out|err) before running the tests to avoid unstable tests.
Use virCommandFDMassClose to close everything except stdio.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/testutils.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/tests/testutils.c b/tests/testutils.c index 35571fb2ad..9ce549370b 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -33,6 +33,7 @@ #include "virbuffer.h" #include "virlog.h" #include "virstring.h" +#include "vircommand.h"
#define VIR_FROM_THIS VIR_FROM_NONE
@@ -840,6 +841,7 @@ int virTestMain(int argc, int (*func)(void), ...) { + g_autoptr(virBitmap) keepfds = virBitmapNew(0); const char *lib; va_list ap; int ret; @@ -859,6 +861,14 @@ int virTestMain(int argc, preloads[npreloads] = NULL; }
+ /* Environment may leak FDs into the test which we don't want to use and + * it could break certain test cases which use 'dup2' to put FDs to correct + * numbers to have stable output. */ + virBitmapSetBitExpand(keepfds, STDIN_FILENO); + virBitmapSetBitExpand(keepfds, STDOUT_FILENO); + virBitmapSetBitExpand(keepfds, STDERR_FILENO); + virCommandFDMassClose(keepfds);
I'm not convinced this is safe to do if it is not followed by an immediate exec.
[1]
Libraries we link to are liable to open file descriptors, either in constructors, or in Libc code that runs before main(). By doing a mass close we are at risk of library code using a bad FD.
Hmmm.
While the tests may pass CI with this today, it feels like a foot-gun waiting to go off.
Yeah. Unfortunately my test cases trying to put FDs into specific places to make test output stable were apparently also loaded foot-guns placed on a shelf waiting to go off. I wonder how to approach this though. I originally didn't want to plumb in the distinction whether the FD passed parts are tested any deeper. Thus I've picked high number FDs expecting that normally opened FDs won't reach that way. Now we do an exec in certain cases (when wanting to preload mocks) which would avoid [1]. Closing FDs only in tests which use mocks would also setup someone for chasing a obscure problem so it would need to be done each time. But the problem is that library constructors can still theoretically use dup2 to take any FD number, so if we want to be able to accomodate any crazy library, another solution will be needed. A simple thing that comes into mind is to add a test-only interface to virCommand allowing to modify the arguments and simply mask out the FD number from any '-add-fd' we encounter. We could then leave tests do natural FD assignments and just mask it in the expected output.
On Wed, May 13, 2026 at 13:19:59 +0200, Peter Krempa via Devel wrote:
On Wed, May 13, 2026 at 11:23:18 +0100, Daniel P. Berrangé wrote:
On Wed, May 13, 2026 at 12:14:56PM +0200, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
Some test cases (such as cases excercising FD passing in qemuxmlconftest) use dup2 to put FDs on certain numbers to have stable test output. If the environment the test program is run in "leaks"/passes some FDs the test may file in a very obscure way.
Since the tests are designed to be standalone we can simply close everything except std(in|out|err) before running the tests to avoid unstable tests.
Use virCommandFDMassClose to close everything except stdio.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/testutils.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/tests/testutils.c b/tests/testutils.c index 35571fb2ad..9ce549370b 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -33,6 +33,7 @@ #include "virbuffer.h" #include "virlog.h" #include "virstring.h" +#include "vircommand.h"
#define VIR_FROM_THIS VIR_FROM_NONE
@@ -840,6 +841,7 @@ int virTestMain(int argc, int (*func)(void), ...) { + g_autoptr(virBitmap) keepfds = virBitmapNew(0); const char *lib; va_list ap; int ret; @@ -859,6 +861,14 @@ int virTestMain(int argc, preloads[npreloads] = NULL; }
+ /* Environment may leak FDs into the test which we don't want to use and + * it could break certain test cases which use 'dup2' to put FDs to correct + * numbers to have stable output. */ + virBitmapSetBitExpand(keepfds, STDIN_FILENO); + virBitmapSetBitExpand(keepfds, STDOUT_FILENO); + virBitmapSetBitExpand(keepfds, STDERR_FILENO); + virCommandFDMassClose(keepfds);
I'm not convinced this is safe to do if it is not followed by an immediate exec.
[1]
Libraries we link to are liable to open file descriptors, either in constructors, or in Libc code that runs before main(). By doing a mass close we are at risk of library code using a bad FD.
Hmmm.
While the tests may pass CI with this today, it feels like a foot-gun waiting to go off.
Yeah. Unfortunately my test cases trying to put FDs into specific places to make test output stable were apparently also loaded foot-guns placed on a shelf waiting to go off.
I wonder how to approach this though. I originally didn't want to plumb in the distinction whether the FD passed parts are tested any deeper.
Thus I've picked high number FDs expecting that normally opened FDs won't reach that way.
Now we do an exec in certain cases (when wanting to preload mocks) which would avoid [1]. Closing FDs only in tests which use mocks would also setup someone for chasing a obscure problem so it would need to be done each time.
But the problem is that library constructors can still theoretically use dup2 to take any FD number, so if we want to be able to accomodate any crazy library, another solution will be needed.
A simple thing that comes into mind is to add a test-only interface to virCommand allowing to modify the arguments and simply mask out the FD number from any '-add-fd' we encounter. We could then leave tests do natural FD assignments and just mask it in the expected output.
And I also wonder if we still want to nuke the FDs (at least when re-execing) even if the solution for the test will be a bit more localized.
On Wed, May 13, 2026 at 01:19:59PM +0200, Peter Krempa wrote:
On Wed, May 13, 2026 at 11:23:18 +0100, Daniel P. Berrangé wrote:
On Wed, May 13, 2026 at 12:14:56PM +0200, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
Some test cases (such as cases excercising FD passing in qemuxmlconftest) use dup2 to put FDs on certain numbers to have stable test output. If the environment the test program is run in "leaks"/passes some FDs the test may file in a very obscure way.
Since the tests are designed to be standalone we can simply close everything except std(in|out|err) before running the tests to avoid unstable tests.
Use virCommandFDMassClose to close everything except stdio.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/testutils.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/tests/testutils.c b/tests/testutils.c index 35571fb2ad..9ce549370b 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -33,6 +33,7 @@ #include "virbuffer.h" #include "virlog.h" #include "virstring.h" +#include "vircommand.h"
#define VIR_FROM_THIS VIR_FROM_NONE
@@ -840,6 +841,7 @@ int virTestMain(int argc, int (*func)(void), ...) { + g_autoptr(virBitmap) keepfds = virBitmapNew(0); const char *lib; va_list ap; int ret; @@ -859,6 +861,14 @@ int virTestMain(int argc, preloads[npreloads] = NULL; }
+ /* Environment may leak FDs into the test which we don't want to use and + * it could break certain test cases which use 'dup2' to put FDs to correct + * numbers to have stable output. */ + virBitmapSetBitExpand(keepfds, STDIN_FILENO); + virBitmapSetBitExpand(keepfds, STDOUT_FILENO); + virBitmapSetBitExpand(keepfds, STDERR_FILENO); + virCommandFDMassClose(keepfds);
I'm not convinced this is safe to do if it is not followed by an immediate exec.
[1]
Libraries we link to are liable to open file descriptors, either in constructors, or in Libc code that runs before main(). By doing a mass close we are at risk of library code using a bad FD.
Hmmm.
While the tests may pass CI with this today, it feels like a foot-gun waiting to go off.
Yeah. Unfortunately my test cases trying to put FDs into specific places to make test output stable were apparently also loaded foot-guns placed on a shelf waiting to go off.
I wonder how to approach this though. I originally didn't want to plumb in the distinction whether the FD passed parts are tested any deeper.
Thus I've picked high number FDs expecting that normally opened FDs won't reach that way.
Now we do an exec in certain cases (when wanting to preload mocks) which would avoid [1]. Closing FDs only in tests which use mocks would also setup someone for chasing a obscure problem so it would need to be done each time.
But the problem is that library constructors can still theoretically use dup2 to take any FD number, so if we want to be able to accomodate any crazy library, another solution will be needed.
A simple thing that comes into mind is to add a test-only interface to virCommand allowing to modify the arguments and simply mask out the FD number from any '-add-fd' we encounter. We could then leave tests do natural FD assignments and just mask it in the expected output.
Yeah, I was wondering if we could mock qemuFDPassTransferCommand to insert a placeholder string instead of whatever FD we got assigned ? With regards, Daniel -- |: https://berrange.com ~~ https://hachyderm.io/@berrange :| |: https://libvirt.org ~~ https://entangle-photo.org :| |: https://pixelfed.art/berrange ~~ https://fstop138.berrange.com :|
participants (2)
-
Daniel P. Berrangé -
Peter Krempa