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

Re: [libvirt] [PATCH] Add missing OOM error checks, reports and cleanups



2009/11/9 Daniel Veillard <veillard redhat com>:
> On Sun, Nov 08, 2009 at 10:40:42PM +0100, Matthias Bolte wrote:
>> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
>> index c2c5a44..c5083cc 100644
>> --- a/src/conf/node_device_conf.c
>> +++ b/src/conf/node_device_conf.c
>> @@ -1057,13 +1057,18 @@ virNodeDeviceDefParseXML(virConnectPtr conn, xmlXPathContextPtr ctxt, int create
>>      /* Extract device name */
>>      if (create == EXISTING_DEVICE) {
>>          def->name = virXPathString(conn, "string(./name[1])", ctxt);
>> +
>> +        if (!def->name) {
>> +            virNodeDeviceReportError(conn, VIR_ERR_NO_NAME, NULL);
>> +            goto error;
>> +        }
>>      } else {
>>          def->name = strdup("new device");
>> -    }
>>
>> -    if (!def->name) {
>> -        virNodeDeviceReportError(conn, VIR_ERR_NO_NAME, NULL);
>> -        goto error;
>> +        if (!def->name) {
>> +            virReportOOMError(conn);
>> +            goto error;
>> +        }
>>      }
>>
>>      /* Extract device parent, if any */
>
>  I disagree with this one. The XPath string(./name[1]) can fail without
> this being an allocation error, it mays just be that there is no name
> element at the current node, and current behaviour looks better to me.
> Moreover if virXPathString() returns NULL because of a string allocation
> error it will already raise an OOMError

I think you misread this one. The original code assigns the result of
virXPathString() or strdup() to def->name. After that it checks
def->name for NULL and reports an no-name error even if the NULL was
returned by strdup(), indicating an OOM error.

I moved the no-name error report into the virXPathString() case and
added an OOM error in the strdup() case.

> [...]
>> diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
>> index ee7a046..c866111 100644
>> --- a/src/remote/remote_driver.c
>> +++ b/src/remote/remote_driver.c
>> @@ -870,12 +870,12 @@ doRemoteOpen (virConnectPtr conn,
>>      }
>>
>>      if(VIR_ALLOC(priv->callbackList)<0) {
>> -        error(conn, VIR_ERR_INVALID_ARG, _("Error allocating callbacks list"));
>> +        virReportOOMError(conn);
>>          goto failed;
>>      }
>>
>>      if(VIR_ALLOC(priv->domainEvents)<0) {
>> -        error(conn, VIR_ERR_INVALID_ARG, _("Error allocating domainEvents"));
>> +        virReportOOMError(conn);
>>          goto failed;
>>      }
>>
>> @@ -2751,9 +2751,18 @@ remoteListDefinedDomains (virConnectPtr conn, char **const names, int maxnames)
>>       * names and the list of pointers, so we have to strdup the
>>       * names here.
>>       */
>> -    for (i = 0; i < ret.names.names_len; ++i)
>> +    for (i = 0; i < ret.names.names_len; ++i) {
>>          names[i] = strdup (ret.names.names_val[i]);
>>
>> +        if (names[i] == NULL) {
>> +            for (--i; i >= 0; --i)
>> +                VIR_FREE(names[i]);
>> +
>> +            virReportOOMError(conn);
>> +            goto cleanup;
>> +        }
>> +    }
>> +
>>      rv = ret.names.names_len;
>>
>>  cleanup:
>> @@ -3086,7 +3095,7 @@ remoteDomainSetSchedulerParameters (virDomainPtr domain,
>>      /* Serialise the scheduler parameters. */
>>      args.params.params_len = nparams;
>>      if (VIR_ALLOC_N(args.params.params_val, nparams) < 0) {
>> -        error (domain->conn, VIR_ERR_RPC, _("out of memory allocating array"));
>> +        virReportOOMError(domain->conn);
>>          goto done;
>>      }
>>
>> @@ -3432,9 +3441,18 @@ remoteListNetworks (virConnectPtr conn, char **const names, int maxnames)
>>       * names and the list of pointers, so we have to strdup the
>>       * names here.
>>       */
>> -    for (i = 0; i < ret.names.names_len; ++i)
>> +    for (i = 0; i < ret.names.names_len; ++i) {
>>          names[i] = strdup (ret.names.names_val[i]);
>>
>> +        if (names[i] == NULL) {
>> +            for (--i; i >= 0; --i)
>> +                VIR_FREE(names[i]);
>> +
>> +            virReportOOMError(conn);
>> +            goto cleanup;
>> +        }
>> +    }
>> +
>>      rv = ret.names.names_len;
>>
>>  cleanup:
>> @@ -3505,9 +3523,18 @@ remoteListDefinedNetworks (virConnectPtr conn,
>>       * names and the list of pointers, so we have to strdup the
>>       * names here.
>>       */
>> -    for (i = 0; i < ret.names.names_len; ++i)
>> +    for (i = 0; i < ret.names.names_len; ++i) {
>>          names[i] = strdup (ret.names.names_val[i]);
>>
>> +        if (names[i] == NULL) {
>> +            for (--i; i >= 0; --i)
>> +                VIR_FREE(names[i]);
>> +
>> +            virReportOOMError(conn);
>> +            goto cleanup;
>> +        }
>> +    }
>> +
>>      rv = ret.names.names_len;
>>
>>  cleanup:
>> @@ -3921,9 +3948,18 @@ remoteListInterfaces (virConnectPtr conn, char **const names, int maxnames)
>>       * names and the list of pointers, so we have to strdup the
>>       * names here.
>>       */
>> -    for (i = 0; i < ret.names.names_len; ++i)
>> +    for (i = 0; i < ret.names.names_len; ++i) {
>>          names[i] = strdup (ret.names.names_val[i]);
>>
>> +        if (names[i] == NULL) {
>> +            for (--i; i >= 0; --i)
>> +                VIR_FREE(names[i]);
>> +
>> +            virReportOOMError(conn);
>> +            goto cleanup;
>> +        }
>> +    }
>> +
>>      rv = ret.names.names_len;
>>
>>  cleanup:
>> @@ -3993,9 +4029,18 @@ remoteListDefinedInterfaces (virConnectPtr conn, char **const names, int maxname
>>       * names and the list of pointers, so we have to strdup the
>>       * names here.
>>       */
>> -    for (i = 0; i < ret.names.names_len; ++i)
>> +    for (i = 0; i < ret.names.names_len; ++i) {
>>          names[i] = strdup (ret.names.names_val[i]);
>>
>> +        if (names[i] == NULL) {
>> +            for (--i; i >= 0; --i)
>> +                VIR_FREE(names[i]);
>> +
>> +            virReportOOMError(conn);
>> +            goto cleanup;
>> +        }
>> +    }
>> +
>>      rv = ret.names.names_len;
>>
>>  cleanup:
>> @@ -4314,9 +4359,18 @@ remoteListStoragePools (virConnectPtr conn, char **const names, int maxnames)
>>       * names and the list of pointers, so we have to strdup the
>>       * names here.
>>       */
>> -    for (i = 0; i < ret.names.names_len; ++i)
>> +    for (i = 0; i < ret.names.names_len; ++i) {
>>          names[i] = strdup (ret.names.names_val[i]);
>>
>> +        if (names[i] == NULL) {
>> +            for (--i; i >= 0; --i)
>> +                VIR_FREE(names[i]);
>> +
>> +            virReportOOMError(conn);
>> +            goto cleanup;
>> +        }
>> +    }
>> +
>>      rv = ret.names.names_len;
>>
>>  cleanup:
>> @@ -4383,9 +4437,18 @@ remoteListDefinedStoragePools (virConnectPtr conn,
>>       * names and the list of pointers, so we have to strdup the
>>       * names here.
>>       */
>> -    for (i = 0; i < ret.names.names_len; ++i)
>> +    for (i = 0; i < ret.names.names_len; ++i) {
>>          names[i] = strdup (ret.names.names_val[i]);
>>
>> +        if (names[i] == NULL) {
>> +            for (--i; i >= 0; --i)
>> +                VIR_FREE(names[i]);
>> +
>> +            virReportOOMError(conn);
>> +            goto cleanup;
>> +        }
>> +    }
>> +
>>      rv = ret.names.names_len;
>>
>>  cleanup:
>> @@ -4890,9 +4953,18 @@ remoteStoragePoolListVolumes (virStoragePoolPtr pool, char **const names, int ma
>>       * names and the list of pointers, so we have to strdup the
>>       * names here.
>>       */
>> -    for (i = 0; i < ret.names.names_len; ++i)
>> +    for (i = 0; i < ret.names.names_len; ++i) {
>>          names[i] = strdup (ret.names.names_val[i]);
>>
>> +        if (names[i] == NULL) {
>> +            for (--i; i >= 0; --i)
>> +                VIR_FREE(names[i]);
>> +
>> +            virReportOOMError(pool->conn);
>> +            goto cleanup;
>> +        }
>> +    }
>> +
>>      rv = ret.names.names_len;
>>
>>  cleanup:
>> @@ -5290,9 +5362,18 @@ static int remoteNodeListDevices(virConnectPtr conn,
>>       * names and the list of pointers, so we have to strdup the
>>       * names here.
>>       */
>> -    for (i = 0; i < ret.names.names_len; ++i)
>> +    for (i = 0; i < ret.names.names_len; ++i) {
>>          names[i] = strdup (ret.names.names_val[i]);
>>
>> +        if (names[i] == NULL) {
>> +            for (--i; i >= 0; --i)
>> +                VIR_FREE(names[i]);
>> +
>> +            virReportOOMError(conn);
>> +            goto cleanup;
>> +        }
>> +    }
>> +
>>      rv = ret.names.names_len;
>>
>>  cleanup:
>> @@ -5443,9 +5524,18 @@ static int remoteNodeDeviceListCaps(virNodeDevicePtr dev,
>>       * names and the list of pointers, so we have to strdup the
>>       * names here.
>>       */
>> -    for (i = 0; i < ret.names.names_len; ++i)
>> +    for (i = 0; i < ret.names.names_len; ++i) {
>>          names[i] = strdup (ret.names.names_val[i]);
>>
>> +        if (names[i] == NULL) {
>> +            for (--i; i >= 0; --i)
>> +                VIR_FREE(names[i]);
>> +
>> +            virReportOOMError(dev->conn);
>> +            goto cleanup;
>> +        }
>> +    }
>> +
>>      rv = ret.names.names_len;
>>
>>  cleanup:
>> @@ -6496,9 +6586,18 @@ remoteSecretListSecrets (virConnectPtr conn, char **uuids, int maxuuids)
>>       * names and the list of pointers, so we have to strdup the
>>       * names here.
>>       */
>> -    for (i = 0; i < ret.uuids.uuids_len; ++i)
>> +    for (i = 0; i < ret.uuids.uuids_len; ++i) {
>>          uuids[i] = strdup (ret.uuids.uuids_val[i]);
>>
>> +        if (uuids[i] == NULL) {
>> +            for (--i; i >= 0; --i)
>> +                VIR_FREE(uuids[i]);
>> +
>> +            virReportOOMError(conn);
>> +            goto cleanup;
>> +        }
>> +    }
>> +
>>      rv = ret.uuids.uuids_len;
>>
>>  cleanup:
>> @@ -6707,8 +6806,10 @@ remoteStreamOpen(virStreamPtr st,
>>      struct private_data *priv = st->conn->privateData;
>>      struct private_stream_data *stpriv;
>>
>> -    if (VIR_ALLOC(stpriv) < 0)
>> +    if (VIR_ALLOC(stpriv) < 0) {
>> +        virReportOOMError(st->conn);
>>          return NULL;
>> +    }
>>
>>      /* Initialize call object used to receive replies */
>>      stpriv->proc_nr = proc_nr;
>> @@ -7106,8 +7207,10 @@ prepareCall(virConnectPtr conn,
>>      struct remote_message_header hdr;
>>      struct remote_thread_call *rv;
>>
>> -    if (VIR_ALLOC(rv) < 0)
>> +    if (VIR_ALLOC(rv) < 0) {
>> +        virReportOOMError(conn);
>>          return NULL;
>> +    }
>>
>>      if (virCondInit(&rv->cond) < 0) {
>>          VIR_FREE(rv);
>> diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
>> index 1d5b4f7..1eef468 100644
>> --- a/src/secret/secret_driver.c
>> +++ b/src/secret/secret_driver.c
>> @@ -449,8 +449,10 @@ secretLoad(virConnectPtr conn, virSecretDriverStatePtr driver,
>>      if (secretLoadValidateUUID(conn, def, xml_basename) < 0)
>>          goto cleanup;
>>
>> -    if (VIR_ALLOC(secret) < 0)
>> +    if (VIR_ALLOC(secret) < 0) {
>> +        virReportOOMError(conn);
>>          goto cleanup;
>> +    }
>>      secret->def = def;
>>      def = NULL;
>>
>> @@ -578,8 +580,10 @@ secretListSecrets(virConnectPtr conn, char **uuids, int maxuuids)
>>          char *uuidstr;
>>          if (i == maxuuids)
>>              break;
>> -        if (VIR_ALLOC_N(uuidstr, VIR_UUID_STRING_BUFLEN) < 0)
>> +        if (VIR_ALLOC_N(uuidstr, VIR_UUID_STRING_BUFLEN) < 0) {
>> +            virReportOOMError(conn);
>>              goto cleanup;
>> +        }
>>          virUUIDFormat(secret->def->uuid, uuidstr);
>>          uuids[i] = uuidstr;
>>          i++;
>
>  Okay, I was just a bit worried about virReportOOMError() in the
>  context of a remote driver entry point but apparently there are
>  previous uses in that context, so nice :-)
>
> [...]
>> @@ -2748,6 +2759,7 @@ cleanup:
>>
>>  struct xenXMListIteratorContext {
>>      virConnectPtr conn;
>> +    int oom;
>>      int max;
>>      int count;
>>      char ** names;
>> @@ -2757,13 +2769,18 @@ static void xenXMListIterator(void *payload ATTRIBUTE_UNUSED, const char *name,
>>      struct xenXMListIteratorContext *ctx = data;
>>      virDomainPtr dom = NULL;
>>
>> +    if (ctx->oom)
>> +        return;
>> +
>>      if (ctx->count == ctx->max)
>>          return;
>>
>>      dom = xenDaemonLookupByName(ctx->conn, name);
>>      if (!dom) {
>> -        ctx->names[ctx->count] = strdup(name);
>> -        ctx->count++;
>> +        if (!(ctx->names[ctx->count] = strdup(name)))
>> +            ctx->oom = 1;
>> +        else
>> +            ctx->count++;
>>      } else {
>>          virDomainFree(dom);
>>      }
>> @@ -2777,7 +2794,7 @@ static void xenXMListIterator(void *payload ATTRIBUTE_UNUSED, const char *name,
>>  int xenXMListDefinedDomains(virConnectPtr conn, char **const names, int maxnames) {
>>      xenUnifiedPrivatePtr priv;
>>      struct xenXMListIteratorContext ctx;
>> -    int ret = -1;
>> +    int i, ret = -1;
>>
>>      if (!VIR_IS_CONNECT(conn)) {
>>          xenXMError(conn, VIR_ERR_INVALID_CONN, __FUNCTION__);
>> @@ -2794,11 +2811,21 @@ int xenXMListDefinedDomains(virConnectPtr conn, char **const names, int maxnames
>>          maxnames = virHashSize(priv->configCache);
>>
>>      ctx.conn = conn;
>> +    ctx.oom = 0;
>>      ctx.count = 0;
>>      ctx.max = maxnames;
>>      ctx.names = names;
>>
>>      virHashForEach(priv->nameConfigMap, xenXMListIterator, &ctx);
>> +
>> +    if (ctx.oom) {
>> +        for (i = 0; i < ctx.count; i++)
>> +            VIR_FREE(ctx.names[i]);
>> +
>> +        virReportOOMError(conn);
>> +        goto cleanup;
>> +    }
>> +
>>      ret = ctx.count;
>>
>>  cleanup:
>
>  So you had to add a filed in the iterator structure to report OOMs
> while running the iterator, nice work !

Well, DPB used this pattern in his "Convert virDomainObjListPtr to use
a hash of domain objects" patch, I just applied it here too :-)

>    ACK except for the one in virNodeDeviceDefParseXML(), very good job
> you must have spent a lot of time, thanks a lot !
>
> Daniel
>

It took some hours, but the task was simple: search and fix.

Matthias


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