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. > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list