Re: [PATCH 2/2] Use more of VIR_XPATH_NODE_AUTORESTORE

[Ján Tomko <jtomko@xxxxxxxxxx>]

On a Wednesday in 2020, Michal Privoznik wrote:
> This is convenience macro, use it more. This commit was generated
> using the following spatch:
>
>  @@
>  symbol node;
>  identifier old;
>  identifier ctxt;
>  type xmlNodePtr;
>  @@
>  - xmlNodePtr old;
>  + VIR_XPATH_NODE_AUTORESTORE(ctxt);
>    ...
>  - old = ctxt->node;
>    ... when != old
>  - ctxt->node = old;
>
>  @@
>  symbol node;
>  identifier old;
>  identifier ctxt;
>  type xmlNodePtr;
>  @@
>  - xmlNodePtr old = ctxt->node;
>  + VIR_XPATH_NODE_AUTORESTORE(ctxt);
>    ... when != old
>  - ctxt->node = old;
>
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
> src/conf/cpu_conf.c             |  3 +-
> src/conf/domain_conf.c          |  7 +--
> src/conf/interface_conf.c       | 16 ++-----
> src/conf/netdev_vlan_conf.c     |  3 +-
> src/conf/network_conf.c         | 25 +++-------
> src/conf/networkcommon_conf.c   |  4 +-
> src/conf/node_device_conf.c     | 84 +++++++++------------------------
> src/conf/numa_conf.c            |  3 +-
> src/conf/snapshot_conf.c        |  3 +-
> src/conf/storage_adapter_conf.c |  3 +-
> src/conf/storage_conf.c         |  4 +-
> src/conf/virsavecookie.c        |  3 +-
> src/cpu/cpu_map.c               | 12 +----
> src/cpu/cpu_x86.c               |  3 +-
> src/lxc/lxc_domain.c            |  4 +-
> src/util/virstorageencryption.c |  8 +---
> src/util/virstoragefile.c       |  3 +-
> tools/virsh-domain.c            |  6 +--
> 18 files changed, 52 insertions(+), 142 deletions(-)

> @@ -797,12 +793,12 @@ virNodeDevCapStorageParseXML(xmlXPathContextPtr ctxt,
>                              xmlNodePtr node,
>                              virNodeDevCapStoragePtr storage)
> {
> -    xmlNodePtr orignode, *nodes = NULL;
> +    VIR_XPATH_NODE_AUTORESTORE(ctxt);
> +    xmlNodePtr *nodes = NULL;
>     size_t i;
>     int n, ret = -1;
>     unsigned long long val;
>
> -    orignode = ctxt->node;
>     ctxt->node = node;
>
>     storage->block = virXPathString("string(./block[1])", ctxt);
> @@ -835,11 +831,8 @@ virNodeDevCapStorageParseXML(xmlXPathContextPtr ctxt,
>         if (STREQ(type, "hotpluggable")) {
>             storage->flags |= VIR_NODE_DEV_CAP_STORAGE_HOTPLUGGABLE;
>         } else if (STREQ(type, "removable")) {
> -            xmlNodePtr orignode2;
> -
>             storage->flags |= VIR_NODE_DEV_CAP_STORAGE_REMOVABLE;
>
> -            orignode2 = ctxt->node;

This set orignode2 to 'node'

>             ctxt->node = nodes[i];
>
>             if (virXPathBoolean("count(./media_available[. = '1']) > 0", ctxt))
> @@ -851,13 +844,10 @@ virNodeDevCapStorageParseXML(xmlXPathContextPtr ctxt,
>             if (virNodeDevCapsDefParseULongLong("number(./media_size[1])", ctxt, &val, def,
>                                                 _("no removable media size supplied for '%s'"),
>                                                 _("invalid removable media size supplied for '%s'")) < 0) {
> -                ctxt->node = orignode2;
>                 VIR_FREE(type);
>                 goto out;
>             }
>             storage->removable_media_size = val;
> -
> -            ctxt->node = orignode2;

This restored it back to 'node'.
The new code can possibly leave it at node[i],
but the code below using the context is specifically run only for
non-removable devices.

My sympathy to anyone touching this code in the future :)

>         } else {
>             virReportError(VIR_ERR_INTERNAL_ERROR,
>                            _("unknown storage capability type '%s' for '%s'"),
> @@ -881,7 +871,6 @@ virNodeDevCapStorageParseXML(xmlXPathContextPtr ctxt,
>     ret = 0;
>  out:
>     VIR_FREE(nodes);
> -    ctxt->node = orignode;
>     return ret;
> }

[...]

> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 61cd72f714..069e3de064 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -6818,7 +6818,7 @@ virshDomainGetVcpuBitmap(vshControl *ctl,
>     xmlDocPtr xml = NULL;
>     xmlXPathContextPtr ctxt = NULL;
>     xmlNodePtr *nodes = NULL;
> -    xmlNodePtr old;
> +    VIR_XPATH_NODE_AUTORESTORE(ctxt);

==1644488== Invalid read of size 8
==1644488==    at 0x169BE9: virshDomainGetVcpuBitmap (virsh-domain.c:6821)
==1644488==    by 0x16965A: virshVcpuinfoInactive (virsh-domain.c:6898)
==1644488==    by 0x160132: cmdVcpuinfo (virsh-domain.c:6970)
==1644488==    by 0x19890E: vshCommandRun (vsh.c:1291)
==1644488==    by 0x1362F9: main (virsh.c:905)
==1644488==  Address 0x8 is not stack'd, malloc'd or (recently) free'd

Since 'ctxt' is freed at the end of the function, there is not much
point in restoring it.

>     int nnodes;
>     size_t i;
>     unsigned int curvcpus = 0;
> @@ -6853,8 +6853,6 @@ virshDomainGetVcpuBitmap(vshControl *ctl,
>         goto cleanup;
>     }
>
> -    old = ctxt->node;
> -
>     for (i = 0; i < nnodes; i++) {
>         ctxt->node = nodes[i];
>
> @@ -6868,8 +6866,6 @@ virshDomainGetVcpuBitmap(vshControl *ctl,
>         VIR_FREE(online);
>     }
>
> -    ctxt->node = old;
> -
>     if (virBitmapCountBits(ret) != curvcpus) {
>         vshError(ctl, "%s", _("Failed to retrieve vcpu state bitmap"));
>         virBitmapFree(ret);

With the crash fixed:

Reviewed-by: Ján Tomko <jtomko@xxxxxxxxxx>

Jano

Attachment: signature.asc
Description: PGP signature