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

Re: [libvirt] [PATCH] Improve invalid argument checks for the public API



On Tue, May 17, 2011 at 02:45:44PM +0200, Matthias Bolte wrote:
> ---
>  src/libvirt.c |  100 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 files changed, 94 insertions(+), 6 deletions(-)


  Damn, that's a lot of missing checks or not ideal reports

> diff --git a/src/libvirt.c b/src/libvirt.c
> index 787908e..87391c6 100644
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -1390,7 +1390,7 @@ int
>  virConnectRef(virConnectPtr conn)
>  {
>      if ((!VIR_IS_CONNECT(conn))) {
> -        virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__);
>          virDispatchError(NULL);
>          return -1;
>      }
> @@ -4487,7 +4487,6 @@ virDomainMigratePrepareTunnel(virConnectPtr conn,
>                                const char *dname,
>                                unsigned long bandwidth,
>                                const char *dom_xml)
> -
>  {
>      VIR_DEBUG("conn=%p, stream=%p, flags=%lu, dname=%s, "
>                "bandwidth=%lu, dom_xml=%s", conn, st, flags,
> @@ -4994,6 +4993,12 @@ virDomainGetSchedulerType(virDomainPtr domain, int *nparams)
>          virDispatchError(NULL);
>          return NULL;
>      }
> +
> +    if (nparams == NULL) {
> +        virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        goto error;
> +    }
> +
>      conn = domain->conn;
>  
>      if (conn->driver->domainGetSchedulerType){
> @@ -5040,6 +5045,12 @@ virDomainGetSchedulerParameters(virDomainPtr domain,
>          virDispatchError(NULL);
>          return -1;
>      }
> +
> +    if (params == NULL || nparams == NULL) {
> +        virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        goto error;
> +    }

  I was just wondering here if params being NULL couldn't be used to
  find out the correct value for nparams, but looking at least at the
  qemu driver code we really expect the array there.

>      conn = domain->conn;
>  
>      if (conn->driver->domainGetSchedulerParameters) {
> @@ -5084,6 +5095,12 @@ virDomainSetSchedulerParameters(virDomainPtr domain,
>          virDispatchError(NULL);
>          return -1;
>      }
> +
> +    if (params == NULL) {
> +        virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        goto error;
> +    }
> +
>      if (domain->conn->flags & VIR_CONNECT_RO) {
>          virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
>          goto error;
> @@ -5534,7 +5551,7 @@ virDomainGetBlockInfo(virDomainPtr domain, const char *path, virDomainBlockInfoP
>          virDispatchError(NULL);
>          return -1;
>      }
> -    if (info == NULL) {
> +    if (path == NULL || info == NULL) {
>          virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
>          goto error;
>      }
> @@ -6440,6 +6457,12 @@ virDomainAttachDevice(virDomainPtr domain, const char *xml)
>          virDispatchError(NULL);
>          return -1;
>      }
> +
> +    if (xml == NULL) {
> +        virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        goto error;
> +    }
> +
>      if (domain->conn->flags & VIR_CONNECT_RO) {
>          virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
>          goto error;
> @@ -6500,6 +6523,12 @@ virDomainAttachDeviceFlags(virDomainPtr domain,
>          virDispatchError(NULL);
>          return -1;
>      }
> +
> +    if (xml == NULL) {
> +        virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        goto error;
> +    }
> +
>      if (domain->conn->flags & VIR_CONNECT_RO) {
>          virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
>          goto error;
> @@ -6545,6 +6574,12 @@ virDomainDetachDevice(virDomainPtr domain, const char *xml)
>          virDispatchError(NULL);
>          return -1;
>      }
> +
> +    if (xml == NULL) {
> +        virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        goto error;
> +    }
> +
>      if (domain->conn->flags & VIR_CONNECT_RO) {
>          virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
>          goto error;
> @@ -6601,6 +6636,12 @@ virDomainDetachDeviceFlags(virDomainPtr domain,
>          virDispatchError(NULL);
>          return -1;
>      }
> +
> +    if (xml == NULL) {
> +        virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        goto error;
> +    }
> +
>      if (domain->conn->flags & VIR_CONNECT_RO) {
>          virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
>          goto error;
> @@ -6661,6 +6702,12 @@ virDomainUpdateDeviceFlags(virDomainPtr domain,
>          virDispatchError(NULL);
>          return -1;
>      }
> +
> +    if (xml == NULL) {
> +        virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        goto error;
> +    }
> +
>      if (domain->conn->flags & VIR_CONNECT_RO) {
>          virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
>          goto error;
> @@ -7321,7 +7368,7 @@ int
>  virNetworkRef(virNetworkPtr network)
>  {
>      if ((!VIR_IS_CONNECTED_NETWORK(network))) {
> -        virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        virLibConnError(VIR_ERR_INVALID_NETWORK, __FUNCTION__);
>          virDispatchError(NULL);
>          return -1;
>      }
> @@ -8187,7 +8234,7 @@ int
>  virInterfaceRef(virInterfacePtr iface)
>  {
>      if ((!VIR_IS_CONNECTED_INTERFACE(iface))) {
> -        virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        virLibConnError(VIR_ERR_INVALID_INTERFACE, __FUNCTION__);
>          virDispatchError(NULL);
>          return -1;
>      }
> @@ -8630,7 +8677,7 @@ virStoragePoolLookupByVolume(virStorageVolPtr vol)
>      virResetLastError();
>  
>      if (!VIR_IS_CONNECTED_STORAGE_VOL(vol)) {
> -        virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        virLibConnError(VIR_ERR_INVALID_STORAGE_VOL, __FUNCTION__);
>          virDispatchError(NULL);
>          return NULL;
>      }
> @@ -9702,6 +9749,11 @@ virStorageVolCreateXML(virStoragePoolPtr pool,
>          return NULL;
>      }
>  
> +    if (xmldesc == NULL) {
> +        virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        goto error;
> +    }
> +
>      if (pool->conn->flags & VIR_CONNECT_RO) {
>          virLibConnError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
>          goto error;
> @@ -9758,6 +9810,11 @@ virStorageVolCreateXMLFrom(virStoragePoolPtr pool,
>          goto error;
>      }
>  
> +    if (xmldesc == NULL) {
> +        virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        goto error;
> +    }
> +
>      if (pool->conn->flags & VIR_CONNECT_RO ||
>          clonevol->conn->flags & VIR_CONNECT_RO) {
>          virLibConnError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
> @@ -10508,6 +10565,11 @@ int virNodeDeviceListCaps(virNodeDevicePtr dev,
>          return -1;
>      }
>  
> +    if (names == NULL || maxnames < 0) {
> +        virLibNodeDeviceError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        goto error;
> +    }
> +
>      if (dev->conn->deviceMonitor && dev->conn->deviceMonitor->deviceListCaps) {
>          int ret;
>          ret = dev->conn->deviceMonitor->deviceListCaps (dev, names, maxnames);
> @@ -11764,6 +11826,11 @@ int virStreamSend(virStreamPtr stream,
>          return -1;
>      }
>  
> +    if (data == NULL) {
> +        virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        goto error;
> +    }
> +
>      if (stream->driver &&
>          stream->driver->streamSend) {
>          int ret;
> @@ -11859,6 +11926,11 @@ int virStreamRecv(virStreamPtr stream,
>          return -1;
>      }
>  
> +    if (data == NULL) {
> +        virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        goto error;
> +    }
> +
>      if (stream->driver &&
>          stream->driver->streamRecv) {
>          int ret;
> @@ -11935,6 +12007,11 @@ int virStreamSendAll(virStreamPtr stream,
>          return -1;
>      }
>  
> +    if (handler == NULL) {
> +        virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        goto cleanup;
> +    }
> +
>      if (stream->flags & VIR_STREAM_NONBLOCK) {
>          virLibConnError(VIR_ERR_OPERATION_INVALID,
>                          _("data sources cannot be used for non-blocking streams"));
> @@ -12032,6 +12109,11 @@ int virStreamRecvAll(virStreamPtr stream,
>          return -1;
>      }
>  
> +    if (handler == NULL) {
> +        virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        goto cleanup;
> +    }
> +
>      if (stream->flags & VIR_STREAM_NONBLOCK) {
>          virLibConnError(VIR_ERR_OPERATION_INVALID,
>                          _("data sinks cannot be used for non-blocking streams"));
> @@ -13746,6 +13828,12 @@ virDomainSnapshotCreateXML(virDomainPtr domain,
>      }
>  
>      conn = domain->conn;
> +
> +    if (xmlDesc == NULL) {
> +        virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        goto error;
> +    }
> +
>      if (conn->flags & VIR_CONNECT_RO) {
>          virLibConnError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
>          goto error;

  ACK, seems all those check are better handled directly in the front end
function

 thanks !

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel veillard com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/


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