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

[Osier Yang]

On 2012年07月30日 22:29, Jiri Denemark wrote:
> 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.
>

Makes sense, ACK.

Osier