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

On Wed, Jun 13, 2018 at 03:11 PM +0200, John Ferlan <jferlan(a)redhat.com&gt; wrote: > On 06/07/2018 08:17 AM, Marc Hartmayer wrote: >> 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. >> > > 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, I used the libvirt-python APIs for some testing. I called an (own) API with 'None' as argument and this resulted in 0 parameters. > 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." This would still require this patch, no? At least if we agree that the following example should work:

virTypedParameterPtr params; int nparams; virTypedParamsDeserialize(NULL, 0, &params, &nparams); assert(params == NULL && assert nparams == 0); Do we?