Re: [libvirt] [PATCH 1/2] virTypedParamsSerialize: minor fixes

On 05/07/2018 11:24 AM, Marc Hartmayer wrote: > On Mon, Apr 30, 2018 at 03:20 PM +0200, John Ferlan <jferlan(a)redhat.com&gt; wrote: >> On 04/25/2018 11:55 AM, Marc Hartmayer wrote: >>> 1. Don't allocate if there is nothing that needs to be >>> allocated. Especially as the result of calling calloc(0, ...) is >>> implementation-defined. uh oh - another memory recollection challenge ;-) >> >> Following VIR_ALLOC_N one finds : >> >> VIR_ALLOC_N(params_val, nparams) >> >> which equates to >> >> # define VIR_ALLOC_N(ptr, count) \ >> virAllocN(&(ptr), sizeof(*(ptr)), (count), true, \ >> VIR_FROM_THIS, __FILE__, __FUNCTION__, __LINE__) >> >> or >> >> virAllocN(&params_val, sizeof(params_val), nparams, true, ...) >> >> and eventually/essentially >> >> *params_val = calloc(sizeof(params_val), nparams) >> >> If the returned value is NULL then we error w/ OOM (4th param=true). >> >> So, unless @params_val had no elements, it won't be calloc(0,...) and > > I'm talking about the case that nparams == 0 => calloc(sizeof(…), 0). > And I was thinking is there a specific consumer/caller of virTypedParamsSerialize that was passing something incorrectly. >> thus the question becomes is there a more specific path you are >> referencing here? > > It’s a “generic” serializer so it should work for every (valid) > case. What happens, for example, if params == NULL and nparams == 0? In > that case I would expect *remote_params_val = NULL and > *remote_params_len = 0. > Going through the callers to virTypedParamsSerialize can/does anyone pass params == NULL and nparams == 0? Would that be reasonable and/or expected? My look didn't find any - either some caller checks that the passed @params != NULL (usually via virCheckNonNullArgGoto in the external API call) or @params is built up on the fly and wouldn't be NULL because there'd be an error causing failure beforehand. Although I'll admit the migration ones are also crazy to look at. I could have made a mistake too; hence, the is there a specific instance that I missed? Or perhaps this is a result of some branched/private code that had that error which I don't have access to?

To answer your "What happens, for example, if params == NULL and nparams == 0?", well supposedly "VIR_ALLOC_N(params_val, 0)" should return NULL, so params_val and thus *remote_params_val == NULL.

Unless of course as Daniel points out needing a patch for bootstrap.conf to use calloc-gnu instead of calloc-posix. John >> >> FWIW: My f26 man page for calloc says: >> >> "The calloc() function allocates memory for an array of nmemb elements >> of size bytes each and returns a pointer to the allocated memory. The >> memory is set to zero. If nmemb or size is 0, then calloc() returns >> either NULL, or a unique pointer value that can later be successfully >> passed to free()" >> >> We have so many places in the code that use VIR_ALLOC_N and do not check >> if the sizeof() or count is 0 - which makes me wonder even more about >> the specific case in which you are referencing. > > It is not about the general use of VIR_ALLOC_N, but about the result of > the serializer and deserializer. > >> I looked through the >> various remote_protocol.x structures that would use this and it seems >> they all use remote_typed_param for the structure being returned, so >> it's not clear how any of them could have a sizeof() returning 0. >> >>> 2. Update the length @remote_params_len only if the related >>> @remote_params_val has also been set. >> >> This one changes the error case as the returned @remote_params_len >> changes from being set to @params_len on input to be undefined. > > Hmm, that’s already the case for 'remote_params_val'? But indeed, we can > either initialize both of them to 0 and NULL or add an error label for > this. > > “@remote_params_len: the final number of elements in > @remote_params_val.” > > […snip…] > > -- > Beste Grüße / Kind regards > Marc Hartmayer > > IBM Deutschland Research & Development GmbH > Vorsitzende des Aufsichtsrats: Martina Koederitz > Geschäftsführung: Dirk Wittkopp > Sitz der Gesellschaft: Böblingen > Registergericht: Amtsgericht Stuttgart, HRB 243294 >