Re: [libvirt] [PATCH v3 3/7] virConnectGetCPUModelNames: implement the remote protocol

On 09/11/2013 08:12 AM, Giuseppe Scrivano wrote: Needs a tweak to pass NULL on to the driver. Wrong variable; here, you want to check if we can encode 'len' into ret. Doesn't work when len is 0 - in that case, models is non-NULL (space to a lone NULL pointer terminating the empty array), but xdr wants us to pass NULL, and we must still free models in that case. Pointless, since you end up assigning all members of args below. What limit? Other functions tell us the limit being violated. Needs a tweak to avoid a blind NULL dereference. Dead assignment; rv is already -1 if we get here. Hmm. The moment we support a NULL models argument, we have to be careful that we free models_val always (not just on error); otherwise, a malicious program on the other end of the rpc pipe could ignore our request for not needing a result and still cause ret to contain an allocated list. I think a cleanup/done pair works for this better than a done/error pair, modelling after the ListAll methods. Seems a bit large given that we return just 2 for i686/x86_64 setups, but no real issue in keeping it that big. Needs a tweak to support the NULL models. ACK with this squashed in: diff --git i/daemon/remote.c w/daemon/remote.c index d357016..c8afa8f 100644 --- i/daemon/remote.c +++ w/daemon/remote.c @@ -5045,7 +5045,7 @@ remoteDispatchConnectGetCPUModelNames(virNetServerPtr server ATTRIBUTE_UNUSED, remote_connect_get_cpu_model_names_ret *ret) { int len, rv = -1; - char **models; + char **models = NULL; struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client); @@ -5054,27 +5054,36 @@ remoteDispatchConnectGetCPUModelNames(virNetServerPtr server ATTRIBUTE_UNUSED, goto cleanup; } - len = virConnectGetCPUModelNames(priv->conn, args->arch, &models, + len = virConnectGetCPUModelNames(priv->conn, args->arch, + args->need_results ? &models : NULL, args->flags); if (len < 0) goto cleanup; - if (ret->models.models_len > REMOTE_CONNECT_CPU_MODELS_MAX) { + if (len > REMOTE_CONNECT_CPU_MODELS_MAX) { virReportError(VIR_ERR_RPC, _("Too many CPU models '%d' for limit '%d'"), - ret->models.models_len, REMOTE_CONNECT_CPU_MODELS_MAX); - virStringFreeList(models); + len, REMOTE_CONNECT_CPU_MODELS_MAX); goto cleanup; } - ret->models.models_val = models; - ret->models.models_len = len; + if (len && models) { + ret->models.models_val = models; + ret->models.models_len = len; + models = NULL; + } else { + ret->models.models_val = NULL; + ret->models.models_len = 0; + } + + ret->ret = len; rv = 0; cleanup: if (rv < 0) virNetMessageSaveError(rerr); + virStringFreeList(models); return rv; } diff --git i/src/remote/remote_driver.c w/src/remote/remote_driver.c index aa3762a..96ccb99 100644 --- i/src/remote/remote_driver.c +++ w/src/remote/remote_driver.c @@ -5548,51 +5548,57 @@ remoteConnectGetCPUModelNames(virConnectPtr conn, { int rv = -1; size_t i; - char **retmodels; + char **retmodels = NULL; remote_connect_get_cpu_model_names_args args; remote_connect_get_cpu_model_names_ret ret; + struct private_data *priv = conn->privateData; - remoteDriverLock(priv); - memset(&args, 0, sizeof(args)); - memset(&ret, 0, sizeof(ret)); + remoteDriverLock(priv); args.arch = (char *) arch; + args.need_results = !!models; args.flags = flags; + memset(&ret, 0, sizeof(ret)); if (call(conn, priv, 0, REMOTE_PROC_CONNECT_GET_CPU_MODEL_NAMES, (xdrproc_t) xdr_remote_connect_get_cpu_model_names_args, (char *) &args, (xdrproc_t) xdr_remote_connect_get_cpu_model_names_ret, (char *) &ret) < 0) - goto error; + goto done; /* Check the length of the returned list carefully. */ if (ret.models.models_len > REMOTE_CONNECT_CPU_MODELS_MAX) { - virReportError(VIR_ERR_RPC, "%s", - _("remoteConnectGetCPUModelNames: " - "returned number of CPU models exceeds limit")); - goto error; + virReportError(VIR_ERR_RPC, + _("Too many model names '%d' for limit '%d'"), + ret.models.models_len, + REMOTE_CONNECT_CPU_MODELS_MAX); + goto cleanup; } - if (VIR_ALLOC_N(retmodels, ret.models.models_len + 1) < 0) - goto error; + if (models) { + if (VIR_ALLOC_N(retmodels, ret.models.models_len + 1) < 0) + goto cleanup; - for (i = 0; i < ret.models.models_len; i++) - retmodels[i] = ret.models.models_val[i]; + for (i = 0; i < ret.models.models_len; i++) { + retmodels[i] = ret.models.models_val[i]; + ret.models.models_val[i] = NULL; + } + *models = retmodels; + retmodels = NULL; + } - /* Caller frees MODELS. */ - *models = retmodels; - rv = ret.models.models_len; + rv = ret.ret; + +cleanup: + virStringFreeList(retmodels); + + xdr_free((xdrproc_t) xdr_remote_connect_get_cpu_model_names_ret, (char *) &ret); done: remoteDriverUnlock(priv); return rv; - -error: - rv = -1; - virStringFreeList(ret.models.models_val); - goto done; } diff --git i/src/remote/remote_protocol.x w/src/remote/remote_protocol.x index bb8b0ce..8d17bad 100644 --- i/src/remote/remote_protocol.x +++ w/src/remote/remote_protocol.x @@ -2840,11 +2840,13 @@ struct remote_domain_event_device_removed_msg { struct remote_connect_get_cpu_model_names_args { remote_nonnull_string arch; + int need_results; unsigned int flags; }; struct remote_connect_get_cpu_model_names_ret { remote_nonnull_string models<REMOTE_CONNECT_CPU_MODELS_MAX>; + int ret; }; /*----- Protocol. -----*/ diff --git i/src/remote_protocol-structs w/src/remote_protocol-structs index 9148202..98d2d5b 100644 --- i/src/remote_protocol-structs +++ w/src/remote_protocol-structs @@ -2318,6 +2318,7 @@ struct remote_domain_event_device_removed_msg { }; struct remote_connect_get_cpu_model_names_args { remote_nonnull_string arch; + int need_results; u_int flags; }; struct remote_connect_get_cpu_model_names_ret { @@ -2325,6 +2326,7 @@ struct remote_connect_get_cpu_model_names_ret { u_int models_len; remote_nonnull_string * models_val; } models; + int ret; }; enum remote_procedure { REMOTE_PROC_CONNECT_OPEN = 1, -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org