xmlCopyNode() may return NULL. Add a check and log an error
in this case.

Found by Linux Verification Center (linuxtesting.org) with Svace.

Fixes: 22766a1a53 ("virshFindDisk: Sanitize use of 'tmp' variable")
Signed-off-by: Anastasia Belova <abelova@astralinux.ru>
---
 tools/virsh-domain.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 7253596f4a..8752294784 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -12875,6 +12875,10 @@ virshFindDisk(const char *doc,
             STREQ_NULLABLE(sourceDir, path) ||
             STREQ_NULLABLE(sourceName, path)) {
             xmlNodePtr ret = xmlCopyNode(nodes[i], 1);
+            if (!ret) {
+                vshError(NULL, "%s", _("Failed to copy node"));
+                return NULL;
+            }
             /* drop backing store since they are not needed here */
             virshDiskDropBackingStore(ret);
             return ret;
-- 
2.43.0

On Wed, Jul 16, 2025 at 16:42:14 +0300, Anastasia Belova wrote:
> xmlCopyNode() may return NULL. Add a check and log an error
> in this case.
> 
> Found by Linux Verification Center (linuxtesting.org) with Svace.
> 
> Fixes: 22766a1a53 ("virshFindDisk: Sanitize use of 'tmp' variable")
> Signed-off-by: Anastasia Belova <abelova@astralinux.ru>
> ---
>  tools/virsh-domain.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 7253596f4a..8752294784 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -12875,6 +12875,10 @@ virshFindDisk(const char *doc,
>              STREQ_NULLABLE(sourceDir, path) ||
>              STREQ_NULLABLE(sourceName, path)) {
>              xmlNodePtr ret = xmlCopyNode(nodes[i], 1);
> +            if (!ret) {
> +                vshError(NULL, "%s", _("Failed to copy node"));

I've changed this to "Failed to copy XML node" as that message is
already used in our code and thus also translated.

> +                return NULL;
> +            }
>              /* drop backing store since they are not needed here */
>              virshDiskDropBackingStore(ret);
>              return ret;
> -- 
> 2.43.0
> 

Reviewed-by: Peter Krempa <pkrempa@redhat.com>

On Thu, Jul 17, 2025 at 12:55:19PM +0200, Peter Krempa via Devel wrote:
> On Wed, Jul 16, 2025 at 16:42:14 +0300, Anastasia Belova wrote:
> > xmlCopyNode() may return NULL. Add a check and log an error
> > in this case.
> > 
> > Found by Linux Verification Center (linuxtesting.org) with Svace.
> > 
> > Fixes: 22766a1a53 ("virshFindDisk: Sanitize use of 'tmp' variable")
> > Signed-off-by: Anastasia Belova <abelova@astralinux.ru>
> > ---
> >  tools/virsh-domain.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> > index 7253596f4a..8752294784 100644
> > --- a/tools/virsh-domain.c
> > +++ b/tools/virsh-domain.c
> > @@ -12875,6 +12875,10 @@ virshFindDisk(const char *doc,
> >              STREQ_NULLABLE(sourceDir, path) ||
> >              STREQ_NULLABLE(sourceName, path)) {
> >              xmlNodePtr ret = xmlCopyNode(nodes[i], 1);
> > +            if (!ret) {
> > +                vshError(NULL, "%s", _("Failed to copy node"));
> 
> I've changed this to "Failed to copy XML node" as that message is
> already used in our code and thus also translated.

FWIW, there are several other examples of xmlCopyNode
return value not being checked across the code.

> 
> > +                return NULL;
> > +            }
> >              /* drop backing store since they are not needed here */
> >              virshDiskDropBackingStore(ret);
> >              return ret;
> > -- 
> > 2.43.0
> > 
> 
> Reviewed-by: Peter Krempa <pkrempa@redhat.com>
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|