[libvirt] [PATCH] qemuTestDriverInit: Don't access live data
Some of our tests (e.g. qemuhotplugtest) call virDomainSaveConfig(). Now the problem is, qemuTestDriverInit() creates a fake qemu driver and fills it with some fake configuration. At least so we hoped. The truth is, it calls regular virQEMUDriverConfigNew() and then fix couple of paths. Literally. Therefore our tests see regular stateDir and configDir for the user that is running the tests. Directories, where live domain XMLs are stored. Let's just hope our test suite hasn't mangled any of them. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/testutilsqemu.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 56a89c913..0726cd317 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -513,6 +513,10 @@ qemuTestParseCapabilities(virCapsPtr caps, void qemuTestDriverFree(virQEMUDriver *driver) { virMutexDestroy(&driver->lock); + if (driver->config) { + virFileDeleteTree(driver->config->stateDir); + virFileDeleteTree(driver->config->configDir); + } virQEMUCapsCacheFree(driver->qemuCapsCache); virObjectUnref(driver->xmlopt); virObjectUnref(driver->caps); @@ -548,9 +552,14 @@ int qemuTestCapsCacheInsert(virQEMUCapsCachePtr cache, const char *binary, return ret; } +# define STATEDIRTEMPLATE abs_builddir "/qemustatedir-XXXXXX" +# define CONFIGDIRTEMPLATE abs_builddir "/qemuconfigdir-XXXXXX" + int qemuTestDriverInit(virQEMUDriver *driver) { virSecurityManagerPtr mgr = NULL; + char statedir[] = STATEDIRTEMPLATE; + char configdir[] = CONFIGDIRTEMPLATE; memset(driver, 0, sizeof(*driver)); @@ -561,6 +570,11 @@ int qemuTestDriverInit(virQEMUDriver *driver) if (!driver->config) goto error; + /* Do this early so that qemuTestDriverFree() doesn't see (unlink) the real + * dirs. */ + VIR_FREE(driver->config->stateDir); + VIR_FREE(driver->config->configDir); + /* Overwrite some default paths so it's consistent for tests. */ VIR_FREE(driver->config->libDir); VIR_FREE(driver->config->channelTargetDir); @@ -568,6 +582,26 @@ int qemuTestDriverInit(virQEMUDriver *driver) VIR_STRDUP(driver->config->channelTargetDir, "/tmp/channel") < 0) goto error; + if (!mkdtemp(statedir)) { + virFilePrintf(stderr, "Cannot create fake stateDir"); + goto error; + } + + if (VIR_STRDUP(driver->config->stateDir, statedir) < 0) { + rmdir(statedir); + goto error; + } + + if (!mkdtemp(configdir)) { + virFilePrintf(stderr, "Cannot create fake configDir"); + goto error; + } + + if (VIR_STRDUP(driver->config->configDir, configdir) < 0) { + rmdir(configdir); + goto error; + } + driver->caps = testQemuCapsInit(); if (!driver->caps) goto error; -- 2.11.0
On Mon, 06 Mar 2017 08:38:00 +0100 Michal Privoznik wrote: <snip> With this patch applied to 3.1.0, 'make check' passes and the build is succesful under fakeroot. Thank you, I owe you a $BEVERAGE_OF_CHOICE :) Pedja
On Mon, Mar 06, 2017 at 08:38:00AM +0100, Michal Privoznik wrote:
Some of our tests (e.g. qemuhotplugtest) call virDomainSaveConfig(). Now the problem is, qemuTestDriverInit() creates a fake qemu driver and fills it with some fake configuration. At least so we hoped. The truth is, it calls regular virQEMUDriverConfigNew() and then fix couple of paths. Literally. Therefore our tests see regular stateDir and configDir for the user that is running the tests. Directories, where live domain XMLs are stored. Let's just hope our test suite hasn't mangled any of them.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/testutilsqemu.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)
diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 56a89c913..0726cd317 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -513,6 +513,10 @@ qemuTestParseCapabilities(virCapsPtr caps, void qemuTestDriverFree(virQEMUDriver *driver) { virMutexDestroy(&driver->lock); + if (driver->config) { + virFileDeleteTree(driver->config->stateDir); + virFileDeleteTree(driver->config->configDir); + } virQEMUCapsCacheFree(driver->qemuCapsCache); virObjectUnref(driver->xmlopt); virObjectUnref(driver->caps); @@ -548,9 +552,14 @@ int qemuTestCapsCacheInsert(virQEMUCapsCachePtr cache, const char *binary, return ret; }
+# define STATEDIRTEMPLATE abs_builddir "/qemustatedir-XXXXXX" +# define CONFIGDIRTEMPLATE abs_builddir "/qemuconfigdir-XXXXXX" + int qemuTestDriverInit(virQEMUDriver *driver) { virSecurityManagerPtr mgr = NULL; + char statedir[] = STATEDIRTEMPLATE; + char configdir[] = CONFIGDIRTEMPLATE;
There's no point in creating these variables, otherwise ACK.
On 03/08/2017 10:15 AM, Martin Kletzander wrote:
On Mon, Mar 06, 2017 at 08:38:00AM +0100, Michal Privoznik wrote:
Some of our tests (e.g. qemuhotplugtest) call virDomainSaveConfig(). Now the problem is, qemuTestDriverInit() creates a fake qemu driver and fills it with some fake configuration. At least so we hoped. The truth is, it calls regular virQEMUDriverConfigNew() and then fix couple of paths. Literally. Therefore our tests see regular stateDir and configDir for the user that is running the tests. Directories, where live domain XMLs are stored. Let's just hope our test suite hasn't mangled any of them.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/testutilsqemu.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)
diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 56a89c913..0726cd317 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -513,6 +513,10 @@ qemuTestParseCapabilities(virCapsPtr caps, void qemuTestDriverFree(virQEMUDriver *driver) { virMutexDestroy(&driver->lock); + if (driver->config) { + virFileDeleteTree(driver->config->stateDir); + virFileDeleteTree(driver->config->configDir); + } virQEMUCapsCacheFree(driver->qemuCapsCache); virObjectUnref(driver->xmlopt); virObjectUnref(driver->caps); @@ -548,9 +552,14 @@ int qemuTestCapsCacheInsert(virQEMUCapsCachePtr cache, const char *binary, return ret; }
+# define STATEDIRTEMPLATE abs_builddir "/qemustatedir-XXXXXX" +# define CONFIGDIRTEMPLATE abs_builddir "/qemuconfigdir-XXXXXX" + int qemuTestDriverInit(virQEMUDriver *driver) { virSecurityManagerPtr mgr = NULL; + char statedir[] = STATEDIRTEMPLATE; + char configdir[] = CONFIGDIRTEMPLATE;
There's no point in creating these variables, otherwise ACK.
There is a point; mkdtemp() uses the passed variable for both input and output. On function enter the file pattern is stored there, on function return the actual dirname that was created is stored there. Calling mkdir(STATEDIRTEMPLATE) would lead to sigsegv I guess. Let me try. Yeah, my guess was right. Pushed without any change. Thank you. Michal
On Wed, Mar 08, 2017 at 10:48:15AM +0100, Michal Privoznik wrote:
On 03/08/2017 10:15 AM, Martin Kletzander wrote:
On Mon, Mar 06, 2017 at 08:38:00AM +0100, Michal Privoznik wrote:
Some of our tests (e.g. qemuhotplugtest) call virDomainSaveConfig(). Now the problem is, qemuTestDriverInit() creates a fake qemu driver and fills it with some fake configuration. At least so we hoped. The truth is, it calls regular virQEMUDriverConfigNew() and then fix couple of paths. Literally. Therefore our tests see regular stateDir and configDir for the user that is running the tests. Directories, where live domain XMLs are stored. Let's just hope our test suite hasn't mangled any of them.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/testutilsqemu.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)
diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 56a89c913..0726cd317 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -513,6 +513,10 @@ qemuTestParseCapabilities(virCapsPtr caps, void qemuTestDriverFree(virQEMUDriver *driver) { virMutexDestroy(&driver->lock); + if (driver->config) { + virFileDeleteTree(driver->config->stateDir); + virFileDeleteTree(driver->config->configDir); + } virQEMUCapsCacheFree(driver->qemuCapsCache); virObjectUnref(driver->xmlopt); virObjectUnref(driver->caps); @@ -548,9 +552,14 @@ int qemuTestCapsCacheInsert(virQEMUCapsCachePtr cache, const char *binary, return ret; }
+# define STATEDIRTEMPLATE abs_builddir "/qemustatedir-XXXXXX" +# define CONFIGDIRTEMPLATE abs_builddir "/qemuconfigdir-XXXXXX" + int qemuTestDriverInit(virQEMUDriver *driver) { virSecurityManagerPtr mgr = NULL; + char statedir[] = STATEDIRTEMPLATE; + char configdir[] = CONFIGDIRTEMPLATE;
There's no point in creating these variables, otherwise ACK.
There is a point; mkdtemp() uses the passed variable for both input and output. On function enter the file pattern is stored there, on function return the actual dirname that was created is stored there. Calling mkdir(STATEDIRTEMPLATE) would lead to sigsegv I guess. Let me try. Yeah, my guess was right.
Pushed without any change. Thank you.
Stupid me, I totally forgot about the mkdtemp(), thanks for understanding.
Michal
participants (3)
-
Martin Kletzander -
Michal Privoznik -
Predrag Ivanovic