• Subject: [libvirt] [PATCH 0/3] Couple of SEV fixes
  • Author: Michal Privoznik
  • Date: June 13, 2018, 10:51 a.m.
  • Patches: 3 / 3
Changeset
src/conf/domain_conf.c  | 30 +++++++++---------------------
src/qemu/qemu_command.c | 25 +++++++++++++++----------
2 files changed, 24 insertions(+), 31 deletions(-)
Git apply log
Switched to a new branch 'cover.1528887096.git.mprivozn@redhat.com'
Applying: qemuBuildSevCommandLine: s/obj/buf/
Applying: qemuBuildSevCommandLine: fix buffer leak
Applying: conf: Rework virDomainSEVDefParseXML()
Using index info to reconstruct a base tree...
M	src/conf/domain_conf.c
Falling back to patching base and 3-way merge...
Auto-merging src/conf/domain_conf.c
CONFLICT (content): Merge conflict in src/conf/domain_conf.c
error: Failed to merge in the changes.
Patch failed at 0001 conf: Rework virDomainSEVDefParseXML()
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Failed to apply patch:
[libvirt] [PATCH 3/3] conf: Rework virDomainSEVDefParseXML()
Test passed: syntax-check

loading

[libvirt] [PATCH 0/3] Couple of SEV fixes
Posted by Michal Privoznik, 1 week ago
*** BLURB HERE ***

Michal Privoznik (3):
  qemuBuildSevCommandLine: s/obj/buf/
  qemuBuildSevCommandLine: fix buffer leak
  conf: Rework virDomainSEVDefParseXML()

 src/conf/domain_conf.c  | 30 +++++++++---------------------
 src/qemu/qemu_command.c | 25 +++++++++++++++----------
 2 files changed, 24 insertions(+), 31 deletions(-)

-- 
2.16.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/3] qemuBuildSevCommandLine: s/obj/buf/
Posted by Michal Privoznik, 1 week ago
The variable points to a buffer not a domain object therefore its
current name is misleading.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_command.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 6a95344fd5..a7f659308c 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9694,7 +9694,7 @@ static int
 qemuBuildSevCommandLine(virDomainObjPtr vm, virCommandPtr cmd,
                         virDomainSevDefPtr sev)
 {
-    virBuffer obj = VIR_BUFFER_INITIALIZER;
+    virBuffer buf = VIR_BUFFER_INITIALIZER;
     qemuDomainObjPrivatePtr priv = vm->privateData;
     char *path = NULL;
 
@@ -9704,25 +9704,25 @@ qemuBuildSevCommandLine(virDomainObjPtr vm, virCommandPtr cmd,
     VIR_DEBUG("policy=0x%x cbitpos=%d reduced_phys_bits=%d",
               sev->policy, sev->cbitpos, sev->reduced_phys_bits);
 
-    virBufferAsprintf(&obj, "sev-guest,id=sev0,cbitpos=%d", sev->cbitpos);
-    virBufferAsprintf(&obj, ",reduced-phys-bits=%d", sev->reduced_phys_bits);
-    virBufferAsprintf(&obj, ",policy=0x%x", sev->policy);
+    virBufferAsprintf(&buf, "sev-guest,id=sev0,cbitpos=%d", sev->cbitpos);
+    virBufferAsprintf(&buf, ",reduced-phys-bits=%d", sev->reduced_phys_bits);
+    virBufferAsprintf(&buf, ",policy=0x%x", sev->policy);
 
     if (sev->dh_cert) {
         if (virAsprintf(&path, "%s/dh_cert.base64", priv->libDir) < 0)
             return -1;
-        virBufferAsprintf(&obj, ",dh-cert-file=%s", path);
+        virBufferAsprintf(&buf, ",dh-cert-file=%s", path);
         VIR_FREE(path);
     }
 
     if (sev->session) {
         if (virAsprintf(&path, "%s/session.base64", priv->libDir) < 0)
             return -1;
-        virBufferAsprintf(&obj, ",session-file=%s", path);
+        virBufferAsprintf(&buf, ",session-file=%s", path);
         VIR_FREE(path);
     }
 
-    virCommandAddArgList(cmd, "-object", virBufferContentAndReset(&obj), NULL);
+    virCommandAddArgList(cmd, "-object", virBufferContentAndReset(&buf), NULL);
     return 0;
 }
 
-- 
2.16.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] qemuBuildSevCommandLine: s/obj/buf/
Posted by Ján Tomko, 1 week ago
On Wed, Jun 13, 2018 at 12:51:57PM +0200, Michal Privoznik wrote:
>The variable points to a buffer not a domain object therefore its
>current name is misleading.
>
>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>---
> src/qemu/qemu_command.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>

Reviewed-by: J�n Tomko <jtomko@redhat.com>

Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/3] qemuBuildSevCommandLine: fix buffer leak
Posted by Michal Privoznik, 1 week ago
The buffer is not freed anywhere. Nor in the error paths. Also
the usage virCommand with respect to buffer is very odd.

==2504== 1,100 bytes in 1 blocks are definitely lost in loss record 167 of 175
==2504==    at 0x4C2CE3F: malloc (vg_replace_malloc.c:298)
==2504==    by 0x4C2F1BF: realloc (vg_replace_malloc.c:785)
==2504==    by 0x5D32EE2: virReallocN (viralloc.c:245)
==2504==    by 0x5D37278: virBufferGrow (virbuffer.c:150)
==2504==    by 0x5D3783E: virBufferVasprintf (virbuffer.c:408)
==2504==    by 0x5D377A9: virBufferAsprintf (virbuffer.c:381)
==2504==    by 0x57017C1: qemuBuildSevCommandLine (qemu_command.c:9707)
==2504==    by 0x57030F7: qemuBuildCommandLine (qemu_command.c:10324)
==2504==    by 0x575FA48: qemuProcessCreatePretendCmd (qemu_process.c:6644)
==2504==    by 0x11351A: testCompareXMLToArgv (qemuxml2argvtest.c:564)
==2504==    by 0x1392F7: virTestRun (testutils.c:180)
==2504==    by 0x137895: mymain (qemuxml2argvtest.c:2900)

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_command.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index a7f659308c..796831f79c 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9697,6 +9697,7 @@ qemuBuildSevCommandLine(virDomainObjPtr vm, virCommandPtr cmd,
     virBuffer buf = VIR_BUFFER_INITIALIZER;
     qemuDomainObjPrivatePtr priv = vm->privateData;
     char *path = NULL;
+    int ret = -1;
 
     if (!sev)
         return 0;
@@ -9710,20 +9711,24 @@ qemuBuildSevCommandLine(virDomainObjPtr vm, virCommandPtr cmd,
 
     if (sev->dh_cert) {
         if (virAsprintf(&path, "%s/dh_cert.base64", priv->libDir) < 0)
-            return -1;
+            goto cleanup;
         virBufferAsprintf(&buf, ",dh-cert-file=%s", path);
         VIR_FREE(path);
     }
 
     if (sev->session) {
         if (virAsprintf(&path, "%s/session.base64", priv->libDir) < 0)
-            return -1;
+            goto cleanup;
         virBufferAsprintf(&buf, ",session-file=%s", path);
         VIR_FREE(path);
     }
 
-    virCommandAddArgList(cmd, "-object", virBufferContentAndReset(&buf), NULL);
-    return 0;
+    virCommandAddArg(cmd, "-object");
+    virCommandAddArgBuffer(cmd, &buf);
+    ret = 0;
+ cleanup:
+    virBufferFreeAndReset(&buf);
+    return ret;
 }
 
 static int
-- 
2.16.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] qemuBuildSevCommandLine: fix buffer leak
Posted by Ján Tomko, 1 week ago
On Wed, Jun 13, 2018 at 12:51:58PM +0200, Michal Privoznik wrote:
>The buffer is not freed anywhere. Nor in the error paths. Also
>the usage virCommand with respect to buffer is very odd.
>
>==2504== 1,100 bytes in 1 blocks are definitely lost in loss record 167 of 175
>==2504==    at 0x4C2CE3F: malloc (vg_replace_malloc.c:298)
>==2504==    by 0x4C2F1BF: realloc (vg_replace_malloc.c:785)
>==2504==    by 0x5D32EE2: virReallocN (viralloc.c:245)
>==2504==    by 0x5D37278: virBufferGrow (virbuffer.c:150)
>==2504==    by 0x5D3783E: virBufferVasprintf (virbuffer.c:408)
>==2504==    by 0x5D377A9: virBufferAsprintf (virbuffer.c:381)
>==2504==    by 0x57017C1: qemuBuildSevCommandLine (qemu_command.c:9707)
>==2504==    by 0x57030F7: qemuBuildCommandLine (qemu_command.c:10324)
>==2504==    by 0x575FA48: qemuProcessCreatePretendCmd (qemu_process.c:6644)
>==2504==    by 0x11351A: testCompareXMLToArgv (qemuxml2argvtest.c:564)
>==2504==    by 0x1392F7: virTestRun (testutils.c:180)
>==2504==    by 0x137895: mymain (qemuxml2argvtest.c:2900)
>
>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>---
> src/qemu/qemu_command.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>

Reviewed-by: J�n Tomko <jtomko@redhat.com>

Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/3] conf: Rework virDomainSEVDefParseXML()
Posted by Michal Privoznik, 1 week ago
Firstly, this function changes node for relative XPaths but
doesn't restore the original one in case VIR_ALLOC(def) fails.
Secondly, @type is leaked. Thirdly, dh-cert and session
attributes are strdup()-ed needlessly, virXPathString already
does that so we can use the retval immediately.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/conf/domain_conf.c | 30 +++++++++---------------------
 1 file changed, 9 insertions(+), 21 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 85f07af46e..c788b525b5 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15849,17 +15849,16 @@ static virDomainSevDefPtr
 virDomainSEVDefParseXML(xmlNodePtr sevNode,
                         xmlXPathContextPtr ctxt)
 {
-    char *tmp = NULL;
     char *type = NULL;
     xmlNodePtr save = ctxt->node;
     virDomainSevDefPtr def;
     unsigned long policy;
 
+    if (VIR_ALLOC(def) < 0)
+        return NULL;
+
     ctxt->node = sevNode;
 
-    if (VIR_ALLOC(def) < 0)
-        return NULL;
-
     if (!(type = virXMLPropString(sevNode, "type"))) {
         virReportError(VIR_ERR_XML_ERROR, "%s",
                        _("missing launch-security type"));
@@ -15899,29 +15898,18 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode,
     }
 
     def->policy = policy;
+    def->dh_cert = virXPathString("string(./dh-cert)", ctxt);
+    def->session = virXPathString("string(./session)", ctxt);
 
-    if ((tmp = virXPathString("string(./dh-cert)", ctxt))) {
-        if (VIR_STRDUP(def->dh_cert, tmp) < 0)
-            goto error;
-
-        VIR_FREE(tmp);
-    }
-
-    if ((tmp = virXPathString("string(./session)", ctxt))) {
-        if (VIR_STRDUP(def->session, tmp) < 0)
-            goto error;
-
-        VIR_FREE(tmp);
-    }
-
+ cleanup:
+    VIR_FREE(type);
     ctxt->node = save;
     return def;
 
  error:
-    VIR_FREE(tmp);
     virDomainSEVDefFree(def);
-    ctxt->node = save;
-    return NULL;
+    def = NULL;
+    goto cleanup;
 }
 
 static virDomainMemoryDefPtr
-- 
2.16.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] conf: Rework virDomainSEVDefParseXML()
Posted by Ján Tomko, 1 week ago
On Wed, Jun 13, 2018 at 12:51:59PM +0200, Michal Privoznik wrote:
>Firstly, this function changes node for relative XPaths but
>doesn't restore the original one in case VIR_ALLOC(def) fails.

This should not matter, since we're going to abort parsing anyway.

>Secondly, @type is leaked. Thirdly, dh-cert and session

s/dh-cert/dhCert

It has been renamed in the meantime.

>attributes are strdup()-ed needlessly, virXPathString already
>does that so we can use the retval immediately.
>
>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>---
> src/conf/domain_conf.c | 30 +++++++++---------------------
> 1 file changed, 9 insertions(+), 21 deletions(-)
>
>diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>index 85f07af46e..c788b525b5 100644
>--- a/src/conf/domain_conf.c
>+++ b/src/conf/domain_conf.c
>@@ -15849,17 +15849,16 @@ static virDomainSevDefPtr
> virDomainSEVDefParseXML(xmlNodePtr sevNode,
>                         xmlXPathContextPtr ctxt)
> {
>-    char *tmp = NULL;
>     char *type = NULL;
>     xmlNodePtr save = ctxt->node;
>     virDomainSevDefPtr def;
>     unsigned long policy;
>
>+    if (VIR_ALLOC(def) < 0)
>+        return NULL;
>+
>     ctxt->node = sevNode;
>
>-    if (VIR_ALLOC(def) < 0)
>-        return NULL;
>-

Or just 'goto error' instead of moving the allocation.
Not sure which option is more future-proof.

>     if (!(type = virXMLPropString(sevNode, "type"))) {
>         virReportError(VIR_ERR_XML_ERROR, "%s",
>                        _("missing launch-security type"));
>@@ -15899,29 +15898,18 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode,
>     }
>
>     def->policy = policy;
>+    def->dh_cert = virXPathString("string(./dh-cert)", ctxt);

string(./dhCert)

>+    def->session = virXPathString("string(./session)", ctxt);
>
>-    if ((tmp = virXPathString("string(./dh-cert)", ctxt))) {
>-        if (VIR_STRDUP(def->dh_cert, tmp) < 0)
>-            goto error;
>-
>-        VIR_FREE(tmp);
>-    }
>-

With the dh-cert -> dhCert adjustments:

Reviewed-by: J�n Tomko <jtomko@redhat.com>

Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list