Re: [libvirt] [PATCH] daemon: Fix crash in virTypedParameterArrayClear

On Mon, Jul 30, 2012 at 21:08:22 +0800, Osier Yang wrote: > On 2012年07月30日 19:55, Jiri Denemark wrote: >> Daemon uses the following pattern when dispatching APIs with typed >> parameters: >> >> VIR_ALLOC_N(params, nparams); >> virDomain*(dom, params,&nparams, flags); >> virTypedParameterArrayClear(params, nparams); >> >> In case nparams was originally set to 0, virDomain* API would fill it >> with the number of typed parameters it can provide and we would use this >> number (rather than zero) to clear params. Because VIR_ALLOC* returns >> non-NULL pointer even if size is 0, the code would end up walking >> through random memory. If we were lucky enough and the memory contained >> 7 (VIR_TYPED_PARAM_STRING) at the right place, we would try to free a >> random pointer and crash. >> >> Let's make sure params stays NULL when nparams is 0. >> --- >> daemon/remote.c | 16 ++++++++-------- >> 1 file changed, 8 insertions(+), 8 deletions(-) >> >> diff --git a/daemon/remote.c b/daemon/remote.c >> index 80626a2..d25717c 100644 >> --- a/daemon/remote.c >> +++ b/daemon/remote.c >> @@ -989,7 +989,7 @@ remoteDispatchDomainGetSchedulerParameters(virNetServerPtr server ATTRIBUTE_UNUS >> virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); >> goto cleanup; >> } >> - if (VIR_ALLOC_N(params, nparams)< 0) >> + if (nparams&& VIR_ALLOC_N(params, nparams)< 0) >> goto no_memory; >> >> if (!(dom = get_nonnull_domain(priv->conn, args->dom))) >> @@ -1098,7 +1098,7 @@ remoteDispatchDomainGetSchedulerParametersFlags(virNetServerPtr server ATTRIBUTE >> virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); >> goto cleanup; >> } >> - if (VIR_ALLOC_N(params, nparams)< 0) >> + if (nparams&& VIR_ALLOC_N(params, nparams)< 0) > > Isn't nparams for these 2 APIs are guarantee positive in the APIs? > > virCheckPositiveArgGoto(*nparams, error); Yes but this function is an entry point from RPC and the daemon should not rely on client-side checking.