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

On Wed, May 09, 2018 at 09:51 PM +0200, John Ferlan <jferlan(a)redhat.com&gt; wrote: > 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? It was the result of private code but actually it was intended and no error :) > > 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. Did you try what 'VIR_ALLOC_N(params_val, 0)' actually returns? At least for me, it doesn’t return a NULL pointer.

The man page for calloc-posix reads: “If either nelem or elsize is 0, then either a null pointer or a unique pointer value that can be successfully passed to free() shall be returned.” [1] [1] https://www.unix.com/man-page/POSIX/3posix/calloc/

> > 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 >> > -- 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