[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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




[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]