[libvirt] [PATCH v1 11/23] locking: Introduce virSeclabelSpace

[John Ferlan]

On 10/12/2015 06:25 AM, Michal Privoznik wrote:
> This module will handle all the security label remembering and
> recalling for virlockd. We need to record a tuple of (path,
> security model, security label). So I'm going with a hash table,
> where @path would be the key, and tuple of (model, label) is
> going to be the payload. Well, the payload tuple is wrapped into
> self inflating array (virSeclabelSpaceLabels). So for each path,
> we will have to traverse through array of various models to find
> the needed one. But since there's gonna be many paths and just a
> few security models, it's going to be the best approach.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  po/POTFILES.in                      |   1 +
>  src/Makefile.am                     |   2 +
>  src/locking/lock_daemon_seclabels.c | 545 ++++++++++++++++++++++++++++++++++++
>  src/locking/lock_daemon_seclabels.h |  43 +++
>  4 files changed, 591 insertions(+)
>  create mode 100644 src/locking/lock_daemon_seclabels.c
>  create mode 100644 src/locking/lock_daemon_seclabels.h
> 

As a "first pass" - I ran this through Coverity and it found a couple
issues...

John

...

> +
> +
> +static void
> +virSeclabelSpaceIterator(void *payload,
> +                         const void *name,
> +                         void *opaque)
> +{
> +    virSeclabelSpaceLabelsPtr labels = payload;
> +    const char *path = name;
> +    struct virSeclabelSpaceIteratorData *data = opaque;
> +    virSeclabelPtr tmp;
> +    virJSONValuePtr item, jsonLabels, jsonLabel;

Coverity found:

jsonLabel and jsonLabels should be initialized since one can jump to
error and call virJSONValueFree before they are properly initialized.

NOTE: Build fails because of this...

> +    size_t i;
> +
> +    if (data->error)
> +        return;
> +
> +    if (!(item = virJSONValueNewObject()))
> +        goto error;
> +
> +    if (virJSONValueObjectAppendString(item, "path", path) < 0)
> +        goto error;
> +
> +    if (!(jsonLabels = virJSONValueNewArray()))
> +        goto error;
> +
> +    for (i = 0; i < labels->nlabels; i++) {
> +        tmp = labels->labels[i];
> +
> +        if (!(jsonLabel = virJSONValueNewObject()))
> +            goto error;
> +
> +        if (virJSONValueObjectAppendString(jsonLabel, "model", tmp->model) < 0 ||
> +            virJSONValueObjectAppendString(jsonLabel, "label", tmp->label) < 0 ||
> +            virJSONValueObjectAppendNumberUint(jsonLabel, "refcount", tmp->refcount) < 0)
> +            goto error;
> +
> +        if (virJSONValueArrayAppend(jsonLabels, jsonLabel) < 0)
> +            goto error;
> +        jsonLabel = NULL;
> +    }
> +
> +    if (virJSONValueObjectAppend(item, "labels", jsonLabels) < 0)
> +        goto error;
> +    jsonLabels = NULL;
> +
> +    if (virJSONValueArrayAppend(data->labels, item) < 0)
> +        goto error;
> +    item = NULL;
> +
> +    return;
> + error:
> +    virJSONValueFree(jsonLabel);
> +    virJSONValueFree(jsonLabels);
> +    virJSONValueFree(item);
> +    data->error = true;
> +}
> +
> +

...

> +/**
> + * virSeclabelSpacePostExecRestart:
> + *
> + * @object: JSON representation of internal state
> + *
> + * Read in JSON object, create new virSeclabelSpace object and restore its internal state from JSON.
> + *
> + * Returns: virSeclabelSpace object
> + *          NULL on error
> + */
> +virSeclabelSpacePtr
> +virSeclabelSpacePostExecRestart(virJSONValuePtr object)
> +{
> +    virSeclabelSpacePtr ret;
> +    virJSONValuePtr labels;
> +    size_t i, npaths;
> +
> +    if (!(ret = virSeclabelSpaceNew()))
> +        return NULL;
> +
> +    if (!(labels = virJSONValueObjectGetArray(object, "seclabels")))
> +        goto error;
> +
> +    npaths = virJSONValueArraySize(labels);

Coverity found:

if npaths is -1, then the following loop ends on an unsigned variable...


> +    for (i = 0; i < npaths; i++) {
> +        virJSONValuePtr item = virJSONValueArrayGet(labels, i);
> +        virJSONValuePtr arr = virJSONValueObjectGetArray(item, "labels");

Coverity found:

if 'arr' is NULL, then deref below won't end well.

> +        const char *path = virJSONValueObjectGetString(item, "path");
> +        size_t j, nlabels;
> +
> +        nlabels = virJSONValueArraySize(arr);

Coverity found:

if nlabels is -1, then the following loop ends on an unsigned variable...

> +
> +        for (j = 0; j < nlabels; j++) {
> +            virJSONValuePtr arr_item = virJSONValueArrayGet(arr, j);
> +            const char *model = virJSONValueObjectGetString(arr_item, "model");
> +            const char *label = virJSONValueObjectGetString(arr_item, "label");
> +            virSeclabelPtr seclabel;
> +
> +            if (!(seclabel = virSeclabelNew(model, label)))
> +                goto error;
> +
> +            if (virJSONValueObjectGetNumberUint(arr_item,
> +                                                "refcount",
> +                                                &seclabel->refcount) < 0) {
> +                virSeclabelFree(seclabel);
> +                goto error;
> +            }
> +
> +            if (virSeclabelSpaceAdd(ret, path, seclabel) < 0) {
> +                virSeclabelFree(seclabel);
> +                goto error;
> +            }
> +        }
> +    }
> +
> +    return ret;
> + error:
> +    virSeclabelSpaceFree(ret);
> +    return NULL;
> +}
> +
> +