[libvirt] [PATCH v3 1/2] Make virSecurityDeviceLabelDefParseXML into generic device <seclabel> parser.

[Michal Privoznik]

On 20.09.2012 17:29, Richard W.M. Jones wrote:
> From: "Richard W.M. Jones" <rjones at redhat.com>
> 
> This is just code motion, allowing us to reuse the same function to
> parse the <seclabel> from character devices too.
> ---
>  src/conf/domain_conf.c |   36 +++++++++++++++++++-----------------
>  1 file changed, 19 insertions(+), 17 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 35814fb..06b3209 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -3253,8 +3253,10 @@ error:
>      return -1;
>  }
>  
> +/* Parse the <seclabel> from a disk or serial device. */
>  static int
> -virSecurityDeviceLabelDefParseXML(virDomainDiskDefPtr def,
> +virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDefPtr **seclabels_rtn,
> +                                  size_t *nseclabels_rtn,
>                                    virSecurityLabelDefPtr *vmSeclabels,
>                                    int nvmSeclabels, xmlXPathContextPtr ctxt)
>  {
> @@ -3263,19 +3265,16 @@ virSecurityDeviceLabelDefParseXML(virDomainDiskDefPtr def,
>      virSecurityLabelDefPtr vmDef = NULL;
>      char *model, *relabel, *label;
>  
> -    if (def == NULL)
> -        return 0;
> -
>      if ((n = virXPathNodeSet("./seclabel", ctxt, &list)) == 0)
>          return 0;
>  
> -    def->nseclabels = n;
> -    if (VIR_ALLOC_N(def->seclabels, n) < 0) {
> +    *nseclabels_rtn = n;
> +    if (VIR_ALLOC_N(*seclabels_rtn, n) < 0) {

I know it is not your fault, but this may lead to SIGSEGV. if this fails
then we proceed to error label where we dereference *seclabels_rtn.

I think the correct solution is:
- if VIR_ALLOC_N() fails, virReportOOMError(); return -1;
- set nseclabels_rtn = n; after alloc succeeds.

or check for *seclabels_rtn != NULL within error label.

>          virReportOOMError();
>          goto error;
>      }
>      for (i = 0; i < n; i++) {
> -        if (VIR_ALLOC(def->seclabels[i]) < 0) {
> +        if (VIR_ALLOC((*seclabels_rtn)[i]) < 0) {
>              virReportOOMError();
>              goto error;
>          }

This is okay, since previous VIR_ALLOC zeroed the allocated memory.

> @@ -3292,7 +3291,7 @@ virSecurityDeviceLabelDefParseXML(virDomainDiskDefPtr def,
>                      break;
>                  }
>              }
> -            def->seclabels[i]->model = model;
> +            (*seclabels_rtn)[i]->model = model;
>          }
>  
>          /* Can't use overrides if top-level doesn't allow relabeling.  */
> @@ -3306,9 +3305,9 @@ virSecurityDeviceLabelDefParseXML(virDomainDiskDefPtr def,
>          relabel = virXMLPropString(list[i], "relabel");
>          if (relabel != NULL) {
>              if (STREQ(relabel, "yes")) {
> -                def->seclabels[i]->norelabel = false;
> +                (*seclabels_rtn)[i]->norelabel = false;
>              } else if (STREQ(relabel, "no")) {
> -                def->seclabels[i]->norelabel = true;
> +                (*seclabels_rtn)[i]->norelabel = true;
>              } else {
>                  virReportError(VIR_ERR_XML_ERROR,
>                                 _("invalid security relabel value %s"),
> @@ -3318,19 +3317,19 @@ virSecurityDeviceLabelDefParseXML(virDomainDiskDefPtr def,
>              }
>              VIR_FREE(relabel);
>          } else {
> -            def->seclabels[i]->norelabel = false;
> +            (*seclabels_rtn)[i]->norelabel = false;
>          }
>  
>          ctxt->node = list[i];
>          label = virXPathStringLimit("string(./label)",
>                                      VIR_SECURITY_LABEL_BUFLEN-1, ctxt);
> -        def->seclabels[i]->label = label;
> +        (*seclabels_rtn)[i]->label = label;
>  
> -        if (label && def->seclabels[i]->norelabel) {
> +        if (label && (*seclabels_rtn)[i]->norelabel) {
>              virReportError(VIR_ERR_XML_ERROR,
>                             _("Cannot specify a label if relabelling is "
>                               "turned off. model=%s"),
> -                             NULLSTR(def->seclabels[i]->model));
> +                           NULLSTR((*seclabels_rtn)[i]->model));
>              goto error;
>          }
>      }
> @@ -3339,10 +3338,11 @@ virSecurityDeviceLabelDefParseXML(virDomainDiskDefPtr def,
>  
>  error:
>      for (i = 0; i < n; i++) {
> -        virSecurityDeviceLabelDefFree(def->seclabels[i]);
> +        virSecurityDeviceLabelDefFree((*seclabels_rtn)[i]);
>      }
> -    VIR_FREE(def->seclabels);
> +    VIR_FREE(*seclabels_rtn);
>      VIR_FREE(list);
> +    *nseclabels_rtn = 0;
>      return -1;
>  }
>  
> @@ -3834,7 +3834,9 @@ virDomainDiskDefParseXML(virCapsPtr caps,
>      if (sourceNode) {
>          xmlNodePtr saved_node = ctxt->node;
>          ctxt->node = sourceNode;
> -        if (virSecurityDeviceLabelDefParseXML(def, vmSeclabels,
> +        if (virSecurityDeviceLabelDefParseXML(&def->seclabels,
> +                                              &def->nseclabels,
> +                                              vmSeclabels,
>                                                nvmSeclabels,
>                                                ctxt) < 0)
>              goto error;
> 

ACK with the issue fixed.

Michal