9:27 p.m.
Since 98b9acf5aa02551dd37d0209339aba2e22e4004a
This was a false positive where Coverity was complaining that the
remoteDeserializeTypedParameters() could allocate 'params', but
none of the callers could return the allocated memory back to their
caller since on input the param was passed by value. Additionally,
the flow of the code was that if params was NULL on entry, then each
function would return 'nparams' as the number of params entries the
caller would need to allocate in order to call the function again
with 'nparams' and 'params' being set. By the time the deserialize
routine was called params would have something. For other callers
where the 'params' was passed by reference as NULL since it's expected
that the deserialize allocates the memory and then have that passed
back to the original caller to dispose there was no Coverity issue.
As it turns out Coverity didn't quite seem to understand the
relationship between 'nparams' and 'params'; however, if the
!userAllocated path of the deserialize code compared against
limit in any manner, then the Coverity error went away which
was quite strange, but useful.
As it turns out one code path remoteDomainGetJobStats had a
comparison against 'limit' while another remoteConnectGetAllDomainStats
did not assuming that limit would be checked. So I refactored the
code a bit to cause the limit check to occur in deserialize for
both conditions and then only made the check of current returned
size against the incoming *nparams fail the non allocation case.
This means the job code doesn't need to check the limit any more,
while the stats code now does check the limit.
Additionally, to help perhaps decipher which of the various
callers to the deserialize code caused the failure - I used
a #define to pass the __FUNCNAME__ of the caller along so that
error messages could have something like:
error: remoteConnectGetAllDomainStats: too many parameters '2' for nparams
'0'
error: Reconnected to the hypervisor
(it's a contrived error just to show the funcname in the error)
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/remote/remote_driver.c | 49 ++++++++++++++++++++++++++--------------------
1 file changed, 28 insertions(+), 21 deletions(-)
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 8221683..4a1b04b 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -72,6 +72,11 @@ VIR_LOG_INIT("remote.remote_driver");
# define HYPER_TO_ULONG(_to, _from) (_to) = (_from)
#endif
+#define remoteDeserializeTypedParameters(ret_params_val, ret_params_len, \
+ limit, params, nparams) \
+ deserializeTypedParameters(__FUNCTION__, ret_params_val, ret_params_len, \
+ limit, params, nparams)
+
static bool inside_daemon = false;
static virDriverPtr remoteDriver = NULL;
@@ -1743,21 +1748,30 @@ remoteSerializeTypedParameters(virTypedParameterPtr params,
/* Helper to deserialize typed parameters. */
static int
-remoteDeserializeTypedParameters(remote_typed_param *ret_params_val,
- u_int ret_params_len,
- int limit,
- virTypedParameterPtr *params,
- int *nparams)
+deserializeTypedParameters(const char *funcname,
+ remote_typed_param *ret_params_val,
+ u_int ret_params_len,
+ int limit,
+ virTypedParameterPtr *params,
+ int *nparams)
{
size_t i = 0;
int rv = -1;
bool userAllocated = *params != NULL;
+ if (ret_params_len > limit) {
+ virReportError(VIR_ERR_RPC,
+ _("%s: too many parameters '%u' for limit
'%d'"),
+ funcname, ret_params_len, limit);
+ goto cleanup;
+ }
+
if (userAllocated) {
/* Check the length of the returned list carefully. */
- if (ret_params_len > limit || ret_params_len > *nparams) {
- virReportError(VIR_ERR_RPC, "%s",
- _("returned number of parameters exceeds limit"));
+ if (ret_params_len > *nparams) {
+ virReportError(VIR_ERR_RPC,
+ _("%s: too many parameters '%u' for nparams
'%d'"),
+ funcname, ret_params_len, *nparams);
goto cleanup;
}
} else {
@@ -1774,8 +1788,8 @@ remoteDeserializeTypedParameters(remote_typed_param
*ret_params_val,
if (virStrcpyStatic(param->field,
ret_param->field) == NULL) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Parameter %s too big for destination"),
- ret_param->field);
+ _("%s: parameter %s too big for destination"),
+ funcname, ret_param->field);
goto cleanup;
}
@@ -1811,8 +1825,8 @@ remoteDeserializeTypedParameters(remote_typed_param
*ret_params_val,
goto cleanup;
break;
default:
- virReportError(VIR_ERR_RPC, _("unknown parameter type: %d"),
- param->type);
+ virReportError(VIR_ERR_RPC, _("%s: unknown parameter type: %d"),
+ funcname, param->type);
goto cleanup;
}
}
@@ -6987,19 +7001,12 @@ remoteDomainGetJobStats(virDomainPtr domain,
(xdrproc_t) xdr_remote_domain_get_job_stats_ret, (char *) &ret) == -1)
goto done;
- if (ret.params.params_len > REMOTE_DOMAIN_JOB_STATS_MAX) {
- virReportError(VIR_ERR_RPC,
- _("Too many job stats '%d' for limit
'%d'"),
- ret.params.params_len,
- REMOTE_DOMAIN_JOB_STATS_MAX);
- goto cleanup;
- }
-
*type = ret.type;
if (remoteDeserializeTypedParameters(ret.params.params_val,
ret.params.params_len,
- 0, params, nparams) < 0)
+ REMOTE_DOMAIN_JOB_STATS_MAX,
+ params, nparams) < 0)
goto cleanup;
rv = 0;
--
1.9.3