3:48 p.m.
Awhile back I noticed that calls to xmlNodeGetContent() from libvirt
code were inconsistent in their handling of the returned
pointer. Sometimes we would assume the return was always non-NULL
(dereferencing with wild abandon without concern for the
consequences), sometimes we would interpret NULL as "", and sometimes
as OOM. I sent mail about this to the list last week, wondering (and
doubting) if we could assume that a NULL return would always mean OOM:
https://www.redhat.com/archives/libvir-list/2020-July/msg00333.html
After looking at the libxml code, danpb's determination was that a
NULL return from xmlNodeGetContent *might* mean OOM, but it might also
mean some odd XML that we weren't expecting, so we can't just always
exit on a NULL return. He also agreed with (and expanded on) my
suspicion that there really is no reliable way to tell the reason for
a NULL return from xmlNodeGetContent, and suggested that possibly we
could just examing the xmlNode directly to learn the content instead
of calling xmlNodeGetContent().
This series is a followup to that discussion. The first 4 patches
clean up the code with the result that:
1) a libvirt wrapper function is always called instead of calling
xmlNodeGetContent() directly.
2) that wrapper function logs an error when it gets back NULL from
xmlNodeGetContent().
3) All the callers check for a NULL return, and do a "silent error
return" themselves when there is a NULL.
In the final patch, I try out Dan's idea of looking at the xmlNode
object directly to get the content. It turns out it's not as
straightforward as you would think from just looking at the layout of
the object - a full explanation is in patch 5. I'm mainly sending that
patch as an "FYI" (one step back from an "RFC"), since really all it
changes is that libvirt will exit on OOM, and log (different, but not
any more informative) error messages when the problem isn't
OOM. Unless someone has a strong opinion otherwise, I think just the
first 4 patches should be applied, and users can just "deal" with the
ambiguity in case of error.
Laine Stump (5):
conf: refactor virDomainBlkioDeviceParseXML to reduce calls to
xmlNodeGetContent
util: replace all calls to xmlNodeGetContent with
virXMLNodeContentString
util: log an error if virXMLNodeContentString will return NULL
treat all NULL returns from virXMLNodeContentString() as an error
util: open code virXMLNodeContentString to access the node object
directly
src/conf/domain_conf.c | 194 ++++++++++++++++++++----------------
src/conf/network_conf.c | 7 +-
src/conf/node_device_conf.c | 6 +-
src/util/virxml.c | 53 +++++++++-
4 files changed, 169 insertions(+), 91 deletions(-)
--
2.25.4
3:48 p.m.
New subject: [PATCH 1/5] conf: refactor virDomainBlkioDeviceParseXML to reduce calls to xmlNodeGetContent
virDomainBlkioDeviceParseXML() calls xmlNodeGetContent() multiple
times in a loop, but can easily be refactored to call it once for all
element nodes, and then use the result of that one call in each of the
(mutually exclusive) blocks that previously each had their own call to
xmlNodeGetContent.
This is being done in order to reduce the number of changes needed in
an upcoming patch that will eliminate the lack of checking for NULL on
return from xmlNodeGetContent().
As part of the simplification, the while() loop has been changed into
a for() so that we can use "continue" without bypassing the "node =
node->next".
Signed-off-by: Laine Stump <laine(a)redhat.com>
Change from V1: turned into for() loop and log error rather than
ignoring NULL. Jano had suggested we might be able to set dev->path
directly instead of using a temporary var, but doing that would
require keeping the error: label and its cleanup of dev->path, rather
than just relying on g_autofree.
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/conf/domain_conf.c | 128 ++++++++++++++++++++++-------------------
1 file changed, 70 insertions(+), 58 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 7ecd2818b9..ade8c13914 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1635,73 +1635,85 @@ virDomainBlkioDeviceParseXML(xmlNodePtr root,
virBlkioDevicePtr dev)
{
xmlNodePtr node;
- g_autofree char *c = NULL;
-
- node = root->children;
- while (node) {
- if (node->type == XML_ELEMENT_NODE) {
- if (virXMLNodeNameEqual(node, "path") && !dev->path) {
- dev->path = (char *)xmlNodeGetContent(node);
- } else if (virXMLNodeNameEqual(node, "weight")) {
- c = (char *)xmlNodeGetContent(node);
- if (virStrToLong_ui(c, NULL, 10, &dev->weight) < 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("could not parse weight %s"),
- c);
- goto error;
- }
- VIR_FREE(c);
- } else if (virXMLNodeNameEqual(node, "read_bytes_sec")) {
- c = (char *)xmlNodeGetContent(node);
- if (virStrToLong_ull(c, NULL, 10, &dev->rbps) < 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("could not parse read bytes sec %s"),
- c);
- goto error;
- }
- VIR_FREE(c);
- } else if (virXMLNodeNameEqual(node, "write_bytes_sec")) {
- c = (char *)xmlNodeGetContent(node);
- if (virStrToLong_ull(c, NULL, 10, &dev->wbps) < 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("could not parse write bytes sec %s"),
- c);
- goto error;
- }
- VIR_FREE(c);
- } else if (virXMLNodeNameEqual(node, "read_iops_sec")) {
- c = (char *)xmlNodeGetContent(node);
- if (virStrToLong_ui(c, NULL, 10, &dev->riops) < 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("could not parse read iops sec %s"),
- c);
- goto error;
- }
- VIR_FREE(c);
- } else if (virXMLNodeNameEqual(node, "write_iops_sec")) {
- c = (char *)xmlNodeGetContent(node);
- if (virStrToLong_ui(c, NULL, 10, &dev->wiops) < 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("could not parse write iops sec %s"),
- c);
- goto error;
- }
- VIR_FREE(c);
+ g_autofree char *path = NULL;
+
+ for (node = root->children; node != NULL; node = node->next) {
+ g_autofree char *c = NULL;
+
+ if (node->type != XML_ELEMENT_NODE)
+ continue;
+
+ c = (char *)xmlNodeGetContent(node);
+
+ if (virXMLNodeNameEqual(node, "path")) {
+ /* To avoid the need for explicit cleanup on failure,
+ * don't set dev->path until we're assured of
+ * success. Until then, store it in an autofree pointer.
+ */
+ if (!path)
+ path = g_steal_pointer(&c);
+ continue;
+ }
+
+ if (virXMLNodeNameEqual(node, "weight")) {
+ if (virStrToLong_ui(c, NULL, 10, &dev->weight) < 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("could not parse weight %s"),
+ c);
+ return -1;
}
+ continue;
+ }
+
+ if (virXMLNodeNameEqual(node, "read_bytes_sec")) {
+ if (virStrToLong_ull(c, NULL, 10, &dev->rbps) < 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("could not parse read bytes sec %s"),
+ c);
+ return -1;
+ }
+ continue;
+ }
+
+ if (virXMLNodeNameEqual(node, "write_bytes_sec")) {
+ if (virStrToLong_ull(c, NULL, 10, &dev->wbps) < 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("could not parse write bytes sec %s"),
+ c);
+ return -1;
+ }
+ continue;
+ }
+
+ if (virXMLNodeNameEqual(node, "read_iops_sec")) {
+ if (virStrToLong_ui(c, NULL, 10, &dev->riops) < 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("could not parse read iops sec %s"),
+ c);
+ return -1;
+ }
+ continue;
+ }
+
+ if (virXMLNodeNameEqual(node, "write_iops_sec")) {
+ if (virStrToLong_ui(c, NULL, 10, &dev->wiops) < 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("could not parse write iops sec %s"),
+ c);
+ return -1;
+ }
+ continue;
}
- node = node->next;
}
- if (!dev->path) {
+
+ if (!path) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("missing per-device path"));
return -1;
}
+ dev->path = g_steal_pointer(&path);
return 0;
-
- error:
- VIR_FREE(dev->path);
- return -1;
}
--
2.25.4
3:48 p.m.
New subject: [PATCH 2/5] util: replace all calls to xmlNodeGetContent with virXMLNodeContentString
No functional change
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/conf/domain_conf.c | 20 ++++++++++----------
src/conf/network_conf.c | 2 +-
src/conf/node_device_conf.c | 4 ++--
3 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index ade8c13914..cb69c97a8e 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1643,7 +1643,7 @@ virDomainBlkioDeviceParseXML(xmlNodePtr root,
if (node->type != XML_ELEMENT_NODE)
continue;
- c = (char *)xmlNodeGetContent(node);
+ c = virXMLNodeContentString(node);
if (virXMLNodeNameEqual(node, "path")) {
/* To avoid the need for explicit cleanup on failure,
@@ -9373,10 +9373,10 @@ virDomainLeaseDefParseXML(xmlNodePtr node)
while (cur != NULL) {
if (cur->type == XML_ELEMENT_NODE) {
if (!key && virXMLNodeNameEqual(cur, "key")) {
- key = (char *)xmlNodeGetContent(cur);
+ key = virXMLNodeContentString(cur);
} else if (!lockspace &&
virXMLNodeNameEqual(cur, "lockspace")) {
- lockspace = (char *)xmlNodeGetContent(cur);
+ lockspace = virXMLNodeContentString(cur);
} else if (!path &&
virXMLNodeNameEqual(cur, "target")) {
path = virXMLPropString(cur, "path");
@@ -10595,16 +10595,16 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
} else if (!serial &&
virXMLNodeNameEqual(cur, "serial")) {
- serial = (char *)xmlNodeGetContent(cur);
+ serial = virXMLNodeContentString(cur);
} else if (!wwn &&
virXMLNodeNameEqual(cur, "wwn")) {
- wwn = (char *)xmlNodeGetContent(cur);
+ wwn = virXMLNodeContentString(cur);
if (!virValidateWWN(wwn))
goto error;
} else if (!vendor &&
virXMLNodeNameEqual(cur, "vendor")) {
- vendor = (char *)xmlNodeGetContent(cur);
+ vendor = virXMLNodeContentString(cur);
if (strlen(vendor) > VENDOR_LEN) {
virReportError(VIR_ERR_XML_ERROR, "%s",
@@ -10619,7 +10619,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
}
} else if (!product &&
virXMLNodeNameEqual(cur, "product")) {
- product = (char *)xmlNodeGetContent(cur);
+ product = virXMLNodeContentString(cur);
if (strlen(product) > PRODUCT_LEN) {
virReportError(VIR_ERR_XML_ERROR, "%s",
@@ -13513,7 +13513,7 @@ virDomainSmartcardDefParseXML(virDomainXMLOptionPtr xmlopt,
"exactly three certificates"));
goto error;
}
- def->data.cert.file[i] = (char *)xmlNodeGetContent(cur);
+ def->data.cert.file[i] = virXMLNodeContentString(cur);
if (!def->data.cert.file[i]) {
virReportOOMError();
goto error;
@@ -13522,7 +13522,7 @@ virDomainSmartcardDefParseXML(virDomainXMLOptionPtr xmlopt,
} else if (cur->type == XML_ELEMENT_NODE &&
virXMLNodeNameEqual(cur, "database") &&
!def->data.cert.database) {
- def->data.cert.database = (char *)xmlNodeGetContent(cur);
+ def->data.cert.database = virXMLNodeContentString(cur);
if (!def->data.cert.database) {
virReportOOMError();
goto error;
@@ -19875,7 +19875,7 @@ virDomainLoaderDefParseXML(xmlNodePtr node,
if (!fwAutoSelect) {
readonly_str = virXMLPropString(node, "readonly");
type_str = virXMLPropString(node, "type");
- loader->path = (char *) xmlNodeGetContent(node);
+ loader->path = virXMLNodeContentString(node);
if (STREQ_NULLABLE(loader->path, ""))
VIR_FREE(loader->path);
}
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 0fd68a7d66..0a32f57188 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -720,7 +720,7 @@ virNetworkDNSHostDefParseXML(const char *networkName,
if (cur->type == XML_ELEMENT_NODE &&
virXMLNodeNameEqual(cur, "hostname")) {
if (cur->children != NULL) {
- g_autofree char *name = (char *) xmlNodeGetContent(cur);
+ g_autofree char *name = virXMLNodeContentString(cur);
if (!name) {
virReportError(VIR_ERR_XML_DETAIL,
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index c54015336a..7bbe709c5d 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -1988,10 +1988,10 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt,
switch ((virNodeDevDevnodeType)val) {
case VIR_NODE_DEV_DEVNODE_DEV:
- def->devnode = (char*)xmlNodeGetContent(node);
+ def->devnode = virXMLNodeContentString(node);
break;
case VIR_NODE_DEV_DEVNODE_LINK:
- def->devlinks[m++] = (char*)xmlNodeGetContent(node);
+ def->devlinks[m++] = virXMLNodeContentString(node);
break;
case VIR_NODE_DEV_DEVNODE_LAST:
break;
--
2.25.4
3:48 p.m.
New subject: [PATCH 3/5] util: log an error if virXMLNodeContentString will return NULL
Many of our calls to xmlNodeGetContent() (which are now all via
virXMLNodeContentString() are failing to check for a NULL return. We
need to remedy that, but in order to make the remedy simpler, let's
log an error in virXMLNodeContentString(), so that the callers don't
all individually need to (since it would be the same error message for
all of them anyway).
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/util/virxml.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/src/util/virxml.c b/src/util/virxml.c
index 27d22598ee..5315d4ff6f 100644
--- a/src/util/virxml.c
+++ b/src/util/virxml.c
@@ -538,7 +538,23 @@ virXMLPropStringLimit(xmlNodePtr node,
char *
virXMLNodeContentString(xmlNodePtr node)
{
- return (char *)xmlNodeGetContent(node);
+ char *ret = (char *)xmlNodeGetContent(node);
+
+ if (node->type != XML_ELEMENT_NODE) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("node '%s' has unexpected type %d"),
+ node->name, node->type);
+ return NULL;
+ }
+
+ if (!ret) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("node '%s' has unexpected NULL content. This could
be caused by malformed input, or a memory allocation failure"),
+ node->name);
+ return NULL;
+ }
+
+ return ret;
}
--
2.25.4
3:48 p.m.
New subject: [PATCH 4/5] treat all NULL returns from virXMLNodeContentString() as an error
and stop erroneously equating NULL with "". The latter means that the
element has empty content, while the former means there was an error
during parsing (either internal with the parser, or the content of the
XML was bad).
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/conf/domain_conf.c | 68 ++++++++++++++++++++++---------------
src/conf/network_conf.c | 5 ++-
src/conf/node_device_conf.c | 6 ++--
3 files changed, 48 insertions(+), 31 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index cb69c97a8e..c377fd74aa 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1643,7 +1643,8 @@ virDomainBlkioDeviceParseXML(xmlNodePtr root,
if (node->type != XML_ELEMENT_NODE)
continue;
- c = virXMLNodeContentString(node);
+ if (!(c = virXMLNodeContentString(node)))
+ return -1;
if (virXMLNodeNameEqual(node, "path")) {
/* To avoid the need for explicit cleanup on failure,
@@ -9373,10 +9374,12 @@ virDomainLeaseDefParseXML(xmlNodePtr node)
while (cur != NULL) {
if (cur->type == XML_ELEMENT_NODE) {
if (!key && virXMLNodeNameEqual(cur, "key")) {
- key = virXMLNodeContentString(cur);
+ if (!(key = virXMLNodeContentString(cur)))
+ goto error;
} else if (!lockspace &&
virXMLNodeNameEqual(cur, "lockspace")) {
- lockspace = virXMLNodeContentString(cur);
+ if (!(lockspace = virXMLNodeContentString(cur)))
+ goto error;
} else if (!path &&
virXMLNodeNameEqual(cur, "target")) {
path = virXMLPropString(cur, "path");
@@ -10595,16 +10598,19 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
} else if (!serial &&
virXMLNodeNameEqual(cur, "serial")) {
- serial = virXMLNodeContentString(cur);
+ if (!(serial = virXMLNodeContentString(cur)))
+ goto error;
} else if (!wwn &&
virXMLNodeNameEqual(cur, "wwn")) {
- wwn = virXMLNodeContentString(cur);
+ if (!(wwn = virXMLNodeContentString(cur)))
+ goto error;
if (!virValidateWWN(wwn))
goto error;
} else if (!vendor &&
virXMLNodeNameEqual(cur, "vendor")) {
- vendor = virXMLNodeContentString(cur);
+ if (!(vendor = virXMLNodeContentString(cur)))
+ goto error;
if (strlen(vendor) > VENDOR_LEN) {
virReportError(VIR_ERR_XML_ERROR, "%s",
@@ -10619,7 +10625,8 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
}
} else if (!product &&
virXMLNodeNameEqual(cur, "product")) {
- product = virXMLNodeContentString(cur);
+ if (!(product = virXMLNodeContentString(cur)))
+ goto error;
if (strlen(product) > PRODUCT_LEN) {
virReportError(VIR_ERR_XML_ERROR, "%s",
@@ -13513,20 +13520,16 @@ virDomainSmartcardDefParseXML(virDomainXMLOptionPtr xmlopt,
"exactly three certificates"));
goto error;
}
- def->data.cert.file[i] = virXMLNodeContentString(cur);
- if (!def->data.cert.file[i]) {
- virReportOOMError();
+ if (!(def->data.cert.file[i] = virXMLNodeContentString(cur)))
goto error;
- }
+
i++;
} else if (cur->type == XML_ELEMENT_NODE &&
virXMLNodeNameEqual(cur, "database") &&
!def->data.cert.database) {
- def->data.cert.database = virXMLNodeContentString(cur);
- if (!def->data.cert.database) {
- virReportOOMError();
+ if (!(def->data.cert.database = virXMLNodeContentString(cur)))
goto error;
- }
+
if (*def->data.cert.database != '/') {
virReportError(VIR_ERR_XML_ERROR,
_("expecting absolute path: %s"),
@@ -15638,8 +15641,10 @@ virSysinfoOEMStringsParseXML(xmlNodePtr node,
goto cleanup;
def->nvalues = nstrings;
- for (i = 0; i < nstrings; i++)
- def->values[i] = virXMLNodeContentString(strings[i]);
+ for (i = 0; i < nstrings; i++) {
+ if (!(def->values[i] = virXMLNodeContentString(strings[i])))
+ goto cleanup;
+ }
*oem = g_steal_pointer(&def);
ret = 0;
@@ -15767,7 +15772,9 @@ virSysinfoParseFWCfgDef(virSysinfoDefPtr def,
return -1;
}
- value = virXMLNodeContentString(nodes[i]);
+ if (!(value = virXMLNodeContentString(nodes[i])))
+ return -1;
+
file = virXMLPropString(nodes[i], "file");
if (virStringIsEmpty(value))
@@ -19875,8 +19882,10 @@ virDomainLoaderDefParseXML(xmlNodePtr node,
if (!fwAutoSelect) {
readonly_str = virXMLPropString(node, "readonly");
type_str = virXMLPropString(node, "type");
- loader->path = virXMLNodeContentString(node);
- if (STREQ_NULLABLE(loader->path, ""))
+ if (!(loader->path = virXMLNodeContentString(node)))
+ return -1;
+
+ if (STREQ(loader->path, ""))
VIR_FREE(loader->path);
}
@@ -20093,14 +20102,15 @@ virDomainVcpuParse(virDomainDefPtr def,
vcpus = maxvcpus = 1;
if ((vcpuNode = virXPathNode("./vcpu[1]", ctxt))) {
- if ((tmp = virXMLNodeContentString(vcpuNode))) {
- if (virStrToLong_ui(tmp, NULL, 10, &maxvcpus) < 0) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("maximum vcpus count must be an integer"));
- return -1;
- }
- VIR_FREE(tmp);
+ if (!(tmp = virXMLNodeContentString(vcpuNode)))
+ return -1;
+
+ if (virStrToLong_ui(tmp, NULL, 10, &maxvcpus) < 0) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("maximum vcpus count must be an integer"));
+ return -1;
}
+ VIR_FREE(tmp);
if ((tmp = virXMLPropString(vcpuNode, "current"))) {
if (virStrToLong_ui(tmp, NULL, 10, &vcpus) < 0) {
@@ -20361,7 +20371,9 @@ virDomainDefParseBootOptions(virDomainDefPtr def,
if (STREQ_NULLABLE(tmp, "slic")) {
VIR_FREE(tmp);
- tmp = virXMLNodeContentString(nodes[0]);
+ if (!(tmp = virXMLNodeContentString(nodes[0])))
+ return -1;
+
def->os.slic_table = virFileSanitizePath(tmp);
} else {
virReportError(VIR_ERR_XML_ERROR,
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 0a32f57188..54a723be64 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -722,7 +722,10 @@ virNetworkDNSHostDefParseXML(const char *networkName,
if (cur->children != NULL) {
g_autofree char *name = virXMLNodeContentString(cur);
- if (!name) {
+ if (!name)
+ goto error;
+
+ if (!name[0]) {
virReportError(VIR_ERR_XML_DETAIL,
_("Missing hostname in network '%s' DNS
HOST record"),
networkName);
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index 7bbe709c5d..31c81a3b85 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -1988,10 +1988,12 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt,
switch ((virNodeDevDevnodeType)val) {
case VIR_NODE_DEV_DEVNODE_DEV:
- def->devnode = virXMLNodeContentString(node);
+ if (!(def->devnode = virXMLNodeContentString(node)))
+ goto error;
break;
case VIR_NODE_DEV_DEVNODE_LINK:
- def->devlinks[m++] = virXMLNodeContentString(node);
+ if (!(def->devlinks[m++] = virXMLNodeContentString(node)))
+ goto error;
break;
case VIR_NODE_DEV_DEVNODE_LAST:
break;
--
2.25.4
3:48 p.m.
New subject: [FYI PATCH 5/5] util: open code virXMLNodeContentString to access the node object directly
(I am *NOT* advocating that we apply this patch. Just providing it for
informational purposes, since we had previously discussed this
possibility on the list)
Since it's impossible to determine whether xmlNodeContent has returned
a NULL due to OOM, or due to badly formed / evil XML, this patch open
codes virXMLNodeContentString to get the content string directly from
the node.
This turns out to not be so easy as it seemed at first glance when it
was suggested - the "content" member of the element node itself does
not contain the content string for the node. The content string that
we want can be found (at least for our uses of libxml) by looking for
a child node of the element node - if that child node is of type
XML_TEXT_NODE, then the content member of *that* node is the string
we're looking for. If there is no child node, then the element has no
content, so we return "". Likewise, if the child node is type
XML_TEXT_NODE but has no content, we also return "". In all other
cases, we log an error and return because this is some case that
hasn't been encountered in our test cases, so either someone is
sending bad XML, or our assumptions about the layout of the XML node
object list are incorrect.
Note that while calling virXMLNodeContentString() would return NULL
from an OOM situation, this new code will exit the process on OOM
(since it is calling glib for memory allocation).
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/util/virxml.c | 43 ++++++++++++++++++++++++++++++++++++++-----
1 file changed, 38 insertions(+), 5 deletions(-)
diff --git a/src/util/virxml.c b/src/util/virxml.c
index 5315d4ff6f..b2298d74c8 100644
--- a/src/util/virxml.c
+++ b/src/util/virxml.c
@@ -538,7 +538,17 @@ virXMLPropStringLimit(xmlNodePtr node,
char *
virXMLNodeContentString(xmlNodePtr node)
{
- char *ret = (char *)xmlNodeGetContent(node);
+ /* We specifically avoid using virXMLNodeContentString() here, because
+ * when NULL is returned, it is difficult/impossible to
+ * distinguish between 1) OOM, 2) NULL content, 3) some other error.
+ */
+
+ /* for elements used the way libvirt uses them, the xmlNode object
+ * for an element will have a type of XML_ELEMENT_NODE, and if the
+ * node has any content, it will be in the content field of a
+ * child node of that object which is itself of type
+ * XML_TEXT_NODE.
+ */
if (node->type != XML_ELEMENT_NODE) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -547,15 +557,38 @@ virXMLNodeContentString(xmlNodePtr node)
return NULL;
}
- if (!ret) {
+ /* no children --> empty element node */
+ if (!node->children)
+ return g_strdup("");
+
+ /* if the child isn't text, or there is more than a single node
+ * hanging off "children", our assumptions have been wrong
+ */
+ if (node->children->type != XML_TEXT_NODE) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("child of element node '%s' has unexpected name
'%s', type %d"),
+ node->name, node->children->name,
node->children->type);
+ return NULL;
+ }
+ if (node->children->next) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("node '%s' has unexpected NULL content. This could
be caused by malformed input, or a memory allocation failure"),
+ _("child of element node '%s' is type XML_TEXT_NODE,
but is a list"),
+ node->name);
+ return NULL;
+ }
+ if (node->children->children) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("child of element node '%s' is type XML_TEXT_NODE,
but has children"),
node->name);
return NULL;
}
- return ret;
-}
+ /* if content is NULL, return "" instead */
+ if (!node->children->content)
+ return g_strdup("");
+
+ return g_strdup((char *)node->children->content);
+ }
/**
--
2.25.4
10:15 a.m.
New subject: [FYI PATCH 5/5] util: open code virXMLNodeContentString to access the node object directly
On 7/20/20 10:48 PM, Laine Stump wrote:
(I am *NOT* advocating that we apply this patch. Just providing it
for
informational purposes, since we had previously discussed this
possibility on the list)
Since it's impossible to determine whether xmlNodeContent has returned
a NULL due to OOM, or due to badly formed / evil XML, this patch open
codes virXMLNodeContentString to get the content string directly from
the node.
This turns out to not be so easy as it seemed at first glance when it
was suggested - the "content" member of the element node itself does
not contain the content string for the node. The content string that
we want can be found (at least for our uses of libxml) by looking for
a child node of the element node - if that child node is of type
XML_TEXT_NODE, then the content member of *that* node is the string
we're looking for. If there is no child node, then the element has no
content, so we return "". Likewise, if the child node is type
XML_TEXT_NODE but has no content, we also return "". In all other
cases, we log an error and return because this is some case that
hasn't been encountered in our test cases, so either someone is
sending bad XML, or our assumptions about the layout of the XML node
object list are incorrect.
Note that while calling virXMLNodeContentString() would return NULL
from an OOM situation, this new code will exit the process on OOM
(since it is calling glib for memory allocation).
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/util/virxml.c | 43 ++++++++++++++++++++++++++++++++++++++-----
1 file changed, 38 insertions(+), 5 deletions(-)
diff --git a/src/util/virxml.c b/src/util/virxml.c
index 5315d4ff6f..b2298d74c8 100644
--- a/src/util/virxml.c
+++ b/src/util/virxml.c
@@ -538,7 +538,17 @@ virXMLPropStringLimit(xmlNodePtr node,
char *
virXMLNodeContentString(xmlNodePtr node)
{
- char *ret = (char *)xmlNodeGetContent(node);
+ /* We specifically avoid using virXMLNodeContentString() here, because
+ * when NULL is returned, it is difficult/impossible to
+ * distinguish between 1) OOM, 2) NULL content, 3) some other error.
+ */
s/virXMLNodeContentString/xmlNodeGetContent/
This patch makes sense to me. I'll leave it up to you whether you merge
it or not.
Michal
10:15 a.m.
On 7/20/20 10:48 PM, Laine Stump wrote:
Awhile back I noticed that calls to xmlNodeGetContent() from libvirt
code were inconsistent in their handling of the returned
pointer. Sometimes we would assume the return was always non-NULL
(dereferencing with wild abandon without concern for the
consequences), sometimes we would interpret NULL as "", and sometimes
as OOM. I sent mail about this to the list last week, wondering (and
doubting) if we could assume that a NULL return would always mean OOM:
https://www.redhat.com/archives/libvir-list/2020-July/msg00333.html
After looking at the libxml code, danpb's determination was that a
NULL return from xmlNodeGetContent *might* mean OOM, but it might also
mean some odd XML that we weren't expecting, so we can't just always
exit on a NULL return. He also agreed with (and expanded on) my
suspicion that there really is no reliable way to tell the reason for
a NULL return from xmlNodeGetContent, and suggested that possibly we
could just examing the xmlNode directly to learn the content instead
of calling xmlNodeGetContent().
This series is a followup to that discussion. The first 4 patches
clean up the code with the result that:
1) a libvirt wrapper function is always called instead of calling
xmlNodeGetContent() directly.
2) that wrapper function logs an error when it gets back NULL from
xmlNodeGetContent().
3) All the callers check for a NULL return, and do a "silent error
return" themselves when there is a NULL.
In the final patch, I try out Dan's idea of looking at the xmlNode
object directly to get the content. It turns out it's not as
straightforward as you would think from just looking at the layout of
the object - a full explanation is in patch 5. I'm mainly sending that
patch as an "FYI" (one step back from an "RFC"), since really all it
changes is that libvirt will exit on OOM, and log (different, but not
any more informative) error messages when the problem isn't
OOM. Unless someone has a strong opinion otherwise, I think just the
first 4 patches should be applied, and users can just "deal" with the
ambiguity in case of error.
Laine Stump (5):
conf: refactor virDomainBlkioDeviceParseXML to reduce calls to
xmlNodeGetContent
util: replace all calls to xmlNodeGetContent with
virXMLNodeContentString
util: log an error if virXMLNodeContentString will return NULL
treat all NULL returns from virXMLNodeContentString() as an error
util: open code virXMLNodeContentString to access the node object
directly
src/conf/domain_conf.c | 194 ++++++++++++++++++++----------------
src/conf/network_conf.c | 7 +-
src/conf/node_device_conf.c | 6 +-
src/util/virxml.c | 53 +++++++++-
4 files changed, 169 insertions(+), 91 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com>
Michal
1613
days inactive
1621
days old
7 comments
2 participants
participants (2)
-
Laine Stump -
Michal Privoznik