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 <[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.
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 :|
