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

On 07/03/2018 06:32 AM, Marc Hartmayer wrote: > On Thu, Jun 21, 2018 at 03:47 PM +0200, Marc Hartmayer <mhartmay(a)linux.ibm.com&gt; wrote: >> 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: > > Okay, the example wouldn’t even compile… but I hope the overall message > is clear :) > >> >> virTypedParameterPtr params; >> int nparams; >> >> virTypedParamsDeserialize(NULL, 0, &params, &nparams); >> assert(params == NULL && assert nparams == 0); >> >> Do we? >> > > Polite ping. > > […ping] > I hope I can politely say I've completely lost context of all of this in my short term memory. :-).

Short answer, not sure. At least not without patch 2 of this series and without checking each called non-remote driver callback function to ensure it handles both 0 nparams and NULL params properly before calling Deserialize.

Still the Deserialize side is documented to handle 2 modes of operation - 1 a two pass model to get the nparams and then call again with a preallocated params buffer and 2 relying on the deserializer for the allocation.

"So far" at least things have been well behaved and I guess I'm still not clear what problem is being chased from "existing" code. You mentioned having "own" code, so perhaps that's where existing assumptions haven't held true. In any case, from my perspective making changes here involves weighing the risks of "fear" over changing algorithms that work and have made some assumption for years against the possible advantage of just not calloc'ing something for the nparams == 0 case.

FWIW: based on subsequent discussion at the very least the commit message would need to be adjusted to indicate calloc(sizeof(...), 0) instead of calloc(0, ...). I think it's been shown that it's not the latter in this case.

John