On 06/07/2018 08:17 AM, Marc Hartmayer wrote: > On Wed, May 09, 2018 at 09:51 PM +0200, John Ferlan <jferlan@xxxxxxxxxx> wrote: >> On 05/07/2018 11:24 AM, Marc Hartmayer wrote: >>> On Mon, Apr 30, 2018 at 03:20 PM +0200, John Ferlan <jferlan@xxxxxxxxxx> 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(¶ms_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. > I think I already determined that NULL isn't returned; otherwise, we would have failed w/ OOM. I do recall trying this and seeing a non NULL value returned. IIRC: I found no way into [de]serialize w/ a 0 parameter, so having that guard didn't seem necessary, but it's been 2 months since I looked at this and that level of detail has long been flushed out of main memory cache. ;-) > 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/ > perhaps then we avoid this conundrum and take Daniel's advice in his response: "If there is any problem with this, then we should just change bootstrap.conf to use calloc-gnu instead of calloc-posix, which basically turns calloc(0) into calloc(1) for compat with glibc behaviour." John >> >> 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 > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
