[PATCH 03/25] conf: refactor virDomainBlkioDeviceParseXML to remove possible NULL dereference

Ján Tomko jtomko at redhat.com
Thu Jun 25 22:44:00 UTC 2020


On a Wednesday in 2020, Laine Stump wrote:
>virDomainBlkioDeviceParseXML() has multiple cases of sending the
>return from xmlNodeGetContent() directly to virStrToLong_xx() without
>checking for NULL. Although it is *very* rare for xmlNodeGetContent()
>to return NULL (possibly it only happens in an OOM condition? The
>documentation is unclear), it could happen, and the refactor in this
>patch manages to eliminate several lines of repeated code while adding
>in a (single) check for NULL.
>
>Signed-off-by: Laine Stump <laine at redhat.com>
>---
> src/conf/domain_conf.c | 39 +++++++++++++++------------------------
> 1 file changed, 15 insertions(+), 24 deletions(-)
>
>diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>index 1916b51d38..8cde1cd0e8 100644
>--- a/src/conf/domain_conf.c
>+++ b/src/conf/domain_conf.c
>@@ -1628,73 +1628,64 @@ virDomainBlkioDeviceParseXML(xmlNodePtr root,
>                              virBlkioDevicePtr dev)
> {
>     xmlNodePtr node;
>-    g_autofree char *c = NULL;
>+    g_autofree char *path = NULL;
>
>     node = root->children;
>     while (node) {
>-        if (node->type == XML_ELEMENT_NODE) {
>-            if (virXMLNodeNameEqual(node, "path") && !dev->path) {
>-                dev->path = (char *)xmlNodeGetContent(node);
>+        g_autofree char *c = NULL;
>+
>+        if (node->type == XML_ELEMENT_NODE &&
>+            (c = (char *)xmlNodeGetContent(node))) {

Missing ErrorReport if xmlNodeGetContent fails.

Converting this open-coded for loop to an actual for loop would
grant us 'continue' privileges, which would make the checks
nicer and give a possibility of assigning the path directly
to 'path', without the extra steal_pointer.

Alternatively, the minimum diff where I'd consider this patch to be
a strict improvement is:

} else if (node->type == XML_ELEMENT_NODE && !c) {
     virReportOOMError();
     return -1;
}

With that: Reviewed-by: Ján Tomko <jtomko at redhat.com>

Jano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20200626/95a7ce0e/attachment-0001.sig>


More information about the libvir-list mailing list