[libvirt] [PATCH v2] conf: prevent crash with no uuid in cephx auth secret

Peter Krempa pkrempa at redhat.com
Mon Dec 3 11:20:57 UTC 2012


On 11/29/12 17:36, Ján Tomko wrote:
> Also remove the pointless check for NULL in auth.cephx.secret.uuid,
> since this is a static array.
> ---
>   src/conf/storage_conf.c |    8 +++-----
>   1 files changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index 3fdc5b6..99c2e52 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -458,7 +458,7 @@ virStoragePoolDefParseAuthCephx(xmlXPathContextPtr ctxt,
>           return -1;
>       }
>
> -    if (virUUIDParse(uuid, auth->secret.uuid) < 0) {
> +    if (uuid && virUUIDParse(uuid, auth->secret.uuid) < 0) {

Given the XML schema that was discussed in the previous version of this 
patch, this hunk is OK, but it doesn't mark the secret that it doesn't 
have an UUID. The old check that was present wasn't effective enough.

With this patch ceph secrets that have just the usage argument will have 
an UUID full of zeroes.

>           virReportError(VIR_ERR_XML_ERROR,
>                          "%s", _("invalid auth secret uuid"));
>           return -1;
> @@ -979,10 +979,8 @@ virStoragePoolSourceFormat(virBufferPtr buf,
>                             src->auth.cephx.username);
>
>           virBufferAsprintf(buf,"      %s", "<secret");
> -        if (src->auth.cephx.secret.uuid != NULL) {
> -            virUUIDFormat(src->auth.cephx.secret.uuid, uuid);
> -            virBufferAsprintf(buf," uuid='%s'", uuid);
> -        }
> +        virUUIDFormat(src->auth.cephx.secret.uuid, uuid);
> +        virBufferAsprintf(buf," uuid='%s'", uuid);

And it will be formated as such here.

>
>           if (src->auth.cephx.secret.usage != NULL) {
>               virBufferAsprintf(buf," usage='%s'", src->auth.cephx.secret.usage);
>

And in virStorageBackendRBDOpenRADOSConn() in 
"src/storage/storage_backend_rbd.c":

if (pool->def->source.auth.cephx.secret.uuid != NULL) {
     virUUIDFormat(pool->def->source.auth.cephx.secret.uuid, secretUuid);
     VIR_DEBUG("Looking up secret by UUID: %s", secretUuid);
     secret = virSecretLookupByUUIDString(conn, secretUuid);
}

if (pool->def->source.auth.cephx.secret.usage != NULL) {
     VIR_DEBUG("Looking up secret by usage: %s",
               pool->def->source.auth.cephx.secret.usage);
     secret = virSecretLookupByUsage(conn, VIR_SECRET_USAGE_TYPE_CEPH,
                             pool->def->source.auth.cephx.secret.usage);
}

If such a secret exists, it will be found by the UUID string at first 
and then overwritten when looked up by usage, this would be OK, if we 
wouldn't have to free the "secret" value afterwards. This causes a memleak.

As a fix, the options (if the schema actually is ok and UUID can be left 
out) that come into my mind are:
1) add a new (bool) field to the secret value holding if UUID was provided
2) converting UUID to a pointer and marking the missing value with NULL 
as it was before.

Option 1 is probably better as you don't have to keep in mind to free 
the UUID array afterwards.

Peter






More information about the libvir-list mailing list