1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH] qemuTestDriverInit: Don't access live data

Michal Privoznik posted 1 patch 7 years, 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/cb6ac82768ef5ea989948b54134357f2977bca93.1488785842.git.mprivozn@redhat.com
tests/testutilsqemu.c | 34 ++++++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
[libvirt] [PATCH] qemuTestDriverInit: Don't access live data
Posted by Michal Privoznik 7 years, 1 month ago 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemuTestDriverInit: Don't access live data
Posted by Predrag Ivanovic 7 years, 1 month ago 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemuTestDriverInit: Don't access live data
Posted by Martin Kletzander 7 years, 1 month ago 
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.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemuTestDriverInit: Don't access live data
Posted by Michal Privoznik 7 years, 1 month ago 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemuTestDriverInit: Don't access live data
Posted by Martin Kletzander 7 years, 1 month ago 
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.