Re: [libvirt] [PATCH v2] conf: prevent crash with no uuid in cephx auth secret
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