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 <[email protected]> > > > > 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 <[email protected]> > > --- > > 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.
