[libvirt] [PATCH v8 2/4] domifaddr: Implement the remote protocol
John Ferlan
jferlan at redhat.com
Fri Feb 13 14:45:00 UTC 2015
On 01/25/2015 01:38 PM, Nehal J Wani wrote:
> daemon/remote.c
> * Define remoteSerializeDomainInterface, remoteDispatchDomainInterfaceAddresses
>
> src/remote/remote_driver.c
> * Define remoteDomainInterfaceAddresses
>
> src/remote/remote_protocol.x
> * New RPC procedure: REMOTE_PROC_DOMAIN_INTERFACE_ADDRESSES
> * Define structs remote_domain_ip_addr, remote_domain_interface,
> remote_domain_interfaces_addresse_args, remote_domain_interface_addresses_ret
> * Introduce upper bounds (to handle DoS attacks):
> REMOTE_DOMAIN_INTERFACE_MAX = 2048
> REMOTE_DOMAIN_IP_ADDR_MAX = 2048
> Restrictions on the maximum number of aliases per interface were
> removed after kernel v2.0, and theoretically, at present, there
> are no upper limits on number of interfaces per virtual machine
> and on the number of IP addresses per interface.
>
> src/remote_protocol-structs
> * New structs added
>
> Signed-off-by: Nehal J Wani <nehaljw.kkd1 at gmail.com>
> ---
> daemon/remote.c | 134 +++++++++++++++++++++++++++++++++++++++++++
> src/libvirt_public.syms | 4 +-
> src/remote/remote_driver.c | 100 ++++++++++++++++++++++++++++++++
> src/remote/remote_protocol.x | 36 +++++++++++-
> src/remote_protocol-structs | 24 ++++++++
> 5 files changed, 295 insertions(+), 3 deletions(-)
>
> diff --git a/daemon/remote.c b/daemon/remote.c
> index d657a09..32b567c 100644
> --- a/daemon/remote.c
> +++ b/daemon/remote.c
> @@ -6438,6 +6438,140 @@ remoteDispatchDomainGetFSInfo(virNetServerPtr server ATTRIBUTE_UNUSED,
> }
>
>
> +static int
> +remoteSerializeDomainInterface(virDomainInterfacePtr *ifaces,
> + unsigned int ifaces_count,
> + remote_domain_interface_addresses_ret *ret)
> +{
> + size_t i, j;
> +
> + if (ifaces_count > REMOTE_DOMAIN_INTERFACE_MAX) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Number of interfaces, %d exceeds the max limit: %d"),
> + ifaces_count, REMOTE_DOMAIN_INTERFACE_MAX);
> + return -1;
> + }
> +
> + if (VIR_ALLOC_N(ret->ifaces.ifaces_val, ifaces_count) < 0)
> + return -1;
Something is not right here... Coverity squawks later on [1]
> +
> + ret->ifaces.ifaces_len = ifaces_count;
> +
> + for (i = 0; i < ifaces_count; i++) {
> + virDomainInterfacePtr iface = ifaces[i];
> + remote_domain_interface *iface_ret = &(ret->ifaces.ifaces_val[i]);
> +
> + if ((VIR_STRDUP(iface_ret->name, iface->name)) < 0)
> + goto cleanup;
> +
> + if (iface->hwaddr) {
> + char **hwaddr_p = NULL;
> + if (VIR_ALLOC(hwaddr_p) < 0)
> + goto cleanup;
> + if (VIR_STRDUP(*hwaddr_p, iface->hwaddr) < 0) {
> + VIR_FREE(hwaddr_p);
> + goto cleanup;
> + }
so hw_addrp is an allocated array of one element of which the [0] is
allocated (strdup'd); however, there's only ever one free...
Should this be something like:
if (iface->hwaddr) {
if (VIR_ALLOC_N(iface_ret->hwaddr, strlen(iface->hwaddr) + 1) < 0)
goto cleanup;
memcpy(iface_ret->hwaddr, iface->hwaddr, strlen(iface->hwaddr));
}
?
> +
> + iface_ret->hwaddr = hwaddr_p;
> + }
> +
> + if (iface->naddrs > REMOTE_DOMAIN_IP_ADDR_MAX) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Number of interfaces, %d exceeds the max limit: %d"),
> + iface->naddrs, REMOTE_DOMAIN_IP_ADDR_MAX);
> + goto cleanup;
> + }
> +
> + if (VIR_ALLOC_N(iface_ret->addrs.addrs_val,
> + iface->naddrs) < 0)
> + goto cleanup;
> +
> + iface_ret->addrs.addrs_len = iface->naddrs;
> +
> + for (j = 0; j < iface->naddrs; j++) {
> + virDomainIPAddressPtr ip_addr = &(iface->addrs[j]);
> + remote_domain_ip_addr *ip_addr_ret =
> + &(iface_ret->addrs.addrs_val[j]);
> +
> + if (VIR_STRDUP(ip_addr_ret->addr, ip_addr->addr) < 0)
> + goto cleanup;
> +
> + ip_addr_ret->prefix = ip_addr->prefix;
> + ip_addr_ret->type = ip_addr->type;
> + }
> + }
> +
> + return 0;
> +
> + cleanup:
> + if (ret->ifaces.ifaces_val) {
> + for (i = 0; i < ifaces_count; i++) {
> + remote_domain_interface *iface_ret = &(ret->ifaces.ifaces_val[i]);
> + VIR_FREE(iface_ret->name);
> + VIR_FREE(iface_ret->hwaddr);
hwaddr was a VIR_ALLOC then VIR_STRDUP(*iface_ret->hwaddr)... so this
would leak the strdup'd string.... unless you change to perhaps _N model.
> + for (j = 0; j < iface_ret->addrs.addrs_len; j++) {
> + remote_domain_ip_addr *ip_addr =
> + &(iface_ret->addrs.addrs_val[j]);
> + VIR_FREE(ip_addr->addr);
> + }
> + VIR_FREE(iface_ret);
^^^[1]
Coverity points out that iface_ret is an incorrect free since it wasn't
allocated separately.... The ifaces_val is an VIR_ALLOC_N, but the
ifaces_val[i] is not.
> + }
> + VIR_FREE(ret->ifaces.ifaces_val);
> + }
> +
> + return -1;
> +}
> +
> +
> +static int
> +remoteDispatchDomainInterfaceAddresses(virNetServerPtr server ATTRIBUTE_UNUSED,
> + virNetServerClientPtr client,
> + virNetMessagePtr msg ATTRIBUTE_UNUSED,
> + virNetMessageErrorPtr rerr,
> + remote_domain_interface_addresses_args *args,
> + remote_domain_interface_addresses_ret *ret)
> +{
> + size_t i;
> + int rv = -1;
> + virDomainPtr dom = NULL;
> + virDomainInterfacePtr *ifaces = NULL;
> + int ifaces_count = 0;
> + struct daemonClientPrivate *priv =
> + virNetServerClientGetPrivateData(client);
> +
> + if (!priv->conn) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
> + goto cleanup;
> + }
> +
> + if (!(dom = get_nonnull_domain(priv->conn, args->dom)))
> + goto cleanup;
> +
> + if ((ifaces_count = virDomainInterfaceAddresses(dom, &ifaces, args->flags)) < 0)
[1] Coverity found this one...
> + goto cleanup;
> +
> + if (remoteSerializeDomainInterface(ifaces, ifaces_count, ret) < 0)
> + goto cleanup;
> +
> + rv = 0;
> +
> + cleanup:
> + if (rv < 0)
> + virNetMessageSaveError(rerr);
> +
> + virObjectUnref(dom);
> +
> + if (ifaces) {
> + for (i = 0; i < ifaces_count; i++)
[1] Coverity doesn't like this because ifaces_count can be negative, but
it doesn't know/follow enough of the path in virDomainInterfaceAddresses
to know they two are related.
Coverity is pacified with the following
if (ifaces_count > 0) {
for (i = 0; i < ifaces_count; i++)
...
> + virDomainInterfaceFree(ifaces[i]);
> + VIR_FREE(ifaces);
> + }
> +
> + return rv;
> +}
> +
> +
> /*----- Helpers. -----*/
>
> /* get_nonnull_domain and get_nonnull_network turn an on-wire
> diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
> index ab86543..52a0a64 100644
> --- a/src/libvirt_public.syms
> +++ b/src/libvirt_public.syms
> @@ -695,10 +695,10 @@ LIBVIRT_1.2.12 {
> virDomainDefineXMLFlags;
> } LIBVIRT_1.2.11;
>
> -LIBVIRT_1.2.12 {
> +LIBVIRT_1.2.13 {
> global:
> virDomainInterfaceAddresses;
> virDomainInterfaceFree;
> -} LIBVIRT_1.2.13;
> +} LIBVIRT_1.2.12;
This hunk needed to be extracted/merged into patch 1
>
> # .... define new API here using predicted next version number ....
> diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
> index 3cc603f..ec0532d 100644
> --- a/src/remote/remote_driver.c
> +++ b/src/remote/remote_driver.c
> @@ -7942,6 +7942,105 @@ remoteDomainGetFSInfo(virDomainPtr dom,
> }
>
>
> +static int
> +remoteDomainInterfaceAddresses(virDomainPtr dom,
> + virDomainInterfacePtr **ifaces,
> + unsigned int flags)
> +{
> + int rv = -1;
> + size_t i, j;
> +
> + virDomainInterfacePtr *ifaces_ret = NULL;
> + remote_domain_interface_addresses_args args;
> + remote_domain_interface_addresses_ret ret;
> +
> + struct private_data *priv = dom->conn->privateData;
> +
> + args.flags = flags;
> + make_nonnull_domain(&args.dom, dom);
> +
> + remoteDriverLock(priv);
> +
> + memset(&ret, 0, sizeof(ret));
> +
> + if (call(dom->conn, priv, 0, REMOTE_PROC_DOMAIN_INTERFACE_ADDRESSES,
> + (xdrproc_t)xdr_remote_domain_interface_addresses_args,
> + (char *)&args,
> + (xdrproc_t)xdr_remote_domain_interface_addresses_ret,
> + (char *)&ret) == -1) {
> + goto done;
> + }
> +
> + if (ret.ifaces.ifaces_len > REMOTE_DOMAIN_INTERFACE_MAX) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Number of interfaces, %d exceeds the max limit: %d"),
> + ret.ifaces.ifaces_len, REMOTE_DOMAIN_INTERFACE_MAX);
> + goto cleanup;
> + }
> +
> + if (ret.ifaces.ifaces_len &&
> + VIR_ALLOC_N(ifaces_ret, ret.ifaces.ifaces_len) < 0)
> + goto cleanup;
> +
> + for (i = 0; i < ret.ifaces.ifaces_len; i++) {
> + if (VIR_ALLOC(ifaces_ret[i]) < 0)
> + goto cleanup;
> +
> + virDomainInterfacePtr iface = ifaces_ret[i];
> + remote_domain_interface *iface_ret = &(ret.ifaces.ifaces_val[i]);
While it's allowed under some compilers - I believe others will complain
about defs after code - eg use the following:
for (i = 0; i < ret.ifaces.ifaces_len; i++)
virDomainInterfacePtr iface;
remote_domain_interface *iface_ret = &(ret.ifaces.ifaces_val[i]);
if (VIR_ALLOC(ifaces_ret[i]) < 0)
goto cleanup;
iface = ifaces_ret[i];
> +
> + if (VIR_STRDUP(iface->name, iface_ret->name) < 0)
> + goto cleanup;
> +
> + if (iface_ret->hwaddr &&
> + VIR_STRDUP(iface->hwaddr, *iface_ret->hwaddr) < 0)
> + goto cleanup;
Note my comment above in remote.c - may change how this is handled.
> +
> + if (iface_ret->addrs.addrs_len > REMOTE_DOMAIN_IP_ADDR_MAX) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Number of interfaces, %d exceeds the max limit: %d"),
> + iface_ret->addrs.addrs_len, REMOTE_DOMAIN_IP_ADDR_MAX);
> + goto cleanup;
> + }
> +
> + iface->naddrs = iface_ret->addrs.addrs_len;
> +
> + if (iface->naddrs) {
> + if (VIR_ALLOC_N(iface->addrs, iface->naddrs) < 0)
> + goto cleanup;
> +
> + for (j = 0; j < iface->naddrs; j++) {
> + virDomainIPAddressPtr ip_addr = &(iface->addrs[j]);
> + remote_domain_ip_addr *ip_addr_ret =
> + &(iface_ret->addrs.addrs_val[j]);
> +
> + if (VIR_STRDUP(ip_addr->addr, ip_addr_ret->addr) < 0)
> + goto cleanup;
> +
> + ip_addr->prefix = ip_addr_ret->prefix;
> + ip_addr->type = ip_addr_ret->type;
> + }
> + }
> + }
> + *ifaces = ifaces_ret;
> + ifaces_ret = NULL;
> +
> + rv = ret.ifaces.ifaces_len;
> +
> + cleanup:
> + if (ifaces_ret) {
> + for (i = 0; i < ret.ifaces.ifaces_len; i++)
> + virDomainInterfaceFree(ifaces_ret[i]);
> + VIR_FREE(ifaces_ret);
> + }
> + xdr_free((xdrproc_t)xdr_remote_domain_interface_addresses_ret,
> + (char *) &ret);
> + done:
> + remoteDriverUnlock(priv);
> + return rv;
> +}
> +
> +
> /* get_nonnull_domain and get_nonnull_network turn an on-wire
> * (name, uuid) pair into virDomainPtr or virNetworkPtr object.
> * These can return NULL if underlying memory allocations fail,
> @@ -8286,6 +8385,7 @@ static virHypervisorDriver hypervisor_driver = {
> .connectGetAllDomainStats = remoteConnectGetAllDomainStats, /* 1.2.8 */
> .nodeAllocPages = remoteNodeAllocPages, /* 1.2.9 */
> .domainGetFSInfo = remoteDomainGetFSInfo, /* 1.2.11 */
> + .domainInterfaceAddresses = remoteDomainInterfaceAddresses, /* 1.2.12 */
1.2.13 (at least)
John
> };
>
> static virNetworkDriver network_driver = {
<...snip...>
More information about the libvir-list
mailing list