Re: [PATCH 6/7] conf: check the return value of virXPathNodeSet

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2012年11月28日 21:34, Ján Tomko wrote:
In a few places, the return value could get passed to VIR_ALLOC_N without
being checked, resulting in a request to allocate a lot of memory if the
return value was negative.

Isn't it expected to fail? calloc fails that on Linux, but not sure what
the behaviour on other platform.

---
  src/conf/domain_conf.c  |    8 ++++++--
  src/conf/storage_conf.c |    4 +++-
  2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 2ca608f..814859a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3258,7 +3258,9 @@ virSecurityLabelDefsParseXML(virDomainDefPtr def,
      saved_node = ctxt->node;

      /* Allocate a security labels based on XML */
-    if ((n = virXPathNodeSet("./seclabel", ctxt,&list)) == 0)
+    if ((n = virXPathNodeSet("./seclabel", ctxt,&list))<  0)
+        goto error;
+    if (n == 0)
          return 0;

but anyway, this is good fix, even calloc will fails, the error just
points to the wrong direction.


      if (VIR_ALLOC_N(def->seclabels, n)<  0) {
@@ -3345,7 +3347,9 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDefPtr **seclabels_rtn,
      virSecurityLabelDefPtr vmDef = NULL;
      char *model, *relabel, *label;

-    if ((n = virXPathNodeSet("./seclabel", ctxt,&list)) == 0)
+    if ((n = virXPathNodeSet("./seclabel", ctxt,&list))<  0)
+        goto error;
+    if (n == 0)
          return 0;

      if (VIR_ALLOC_N(seclabels, n)<  0) {
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 5a2cf1b..feeba17 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -510,7 +510,9 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt,
          VIR_FREE(format);
      }

-    source->nhost = virXPathNodeSet("./host", ctxt,&nodeset);
+    if ((i = virXPathNodeSet("./host", ctxt,&nodeset))<  0)
+        goto cleanup;
+    source->nhost = i;

I'd like have a new var like "n", instead of reusing "i", which
is used for loop index in convention.


      if (source->nhost) {
          if (VIR_ALLOC_N(source->hosts, source->nhost)<  0) {

ACK. I will use "n" when pushing.

Osier

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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]