On 09/11/2014 11:55 AM, Eric Blake wrote:
On 09/11/2014 02:38 AM, Peter Krempa wrote:
On 09/05/14 00:26, John Ferlan wrote:
Since 98b9acf5aa02551dd37d0209339aba2e22e4004a
This ends up being a false positive for two reasons...
expected to be already allocated and thus is passed by value; whereas, the call into remoteDomainGetJobStats() 'params' is passed by reference. Thus if the VIR_ALLOC is done there is no way for it to be leaked for callers that passed by value.
path that handles 'nparams == 0 && params == NULL' on entry. Thus all
Careful, my virDomainBlockCopy API passes nparams==0 && params == (non-NULL 0-length array) from the RPC daemon/remote.c receiver into the libvirtd side of the API call. It tripped me up the first time I tried to assert that nparams==0 implied params==NULL, and failed the test.
Well in general the newer Coverity has been squawking about this case - that is assumptions where nparams/params type parameters are used. In most cases, it's a "for (i = 0; i < nparams; i++)" do something with "params[i]" that get flagged on the params[i] reference because there's no nparams == 0 type check. Particular to this query though the code in question is remoteDeserializeTypedParameters(); whereas, the remoteDomainBlockCopy() uses remoteSerializeTypedParameters(). And yes, it's all very tricky. While I do believe from reading the code that this particular case is a false positive - I really was trying to find a way around making too many changes. Open to suggestions naturally! John
other callers have guaranteed that 'params' is non NULL. Of course Coverity isn't wise enough to pick up on this, but nonetheless is does point out something "future callers" for which future callers need to be aware.
Even though it is a false positive, it's probably a good idea to at least make some sort of check (and to "trick" Coverity into believing we know what we're doing). The easiest/cheapest way was to check the input 'limit' value. For the remoteDomainGetJobStats() it is passed as 0 indicating (perhaps) that the caller has done the limits length checking already and that its caller can handle allocating something that can be passed back to the caller.
This unfortunately breaks the remote driver impl of GetAllDomainStats. As it seems that the limit parameter isn't used for automatically allocated arrays and I expected that it is I'll need either fix the remote impl of the stats function or add support for checking the limit here. And I probably prefer option 2, fixing remoteDeserializeTypedParameters to use limit correctly even for auto-alloced typed parameters.
I haven't looked closely at the patch, but it is a tricky area of code.