On Wed, Jul 04, 2018 at 11:24 AM +0200, Marc Hartmayer <mhartmay@xxxxxxxxxxxxx> wrote: > On Tue, Jul 03, 2018 at 09:14 PM +0200, John Ferlan <jferlan@xxxxxxxxxx> wrote: >> On 07/03/2018 06:32 AM, Marc Hartmayer wrote: >>> On Thu, Jun 21, 2018 at 03:47 PM +0200, Marc Hartmayer <mhartmay@xxxxxxxxxxxxx> wrote: >>>> On Wed, Jun 13, 2018 at 03:11 PM +0200, John Ferlan <jferlan@xxxxxxxxxx> wrote: >>>>> 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, >>>> >>>> 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, ¶ms, &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. :-). > > Yep, understand that :) > >> >> 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. > > Which callback functions do you mean? > >> >> 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. > > Can you please give me an example where the deserializer is called > twice? And what is meant with “deserializer”? The function > virTypedParamsDeserialize? > > For qemuDomainGetBlkioParameters/remoteDomainGetBlkioParameters, for > example, not virTypedParamsDeserialize returns nparams - instead it’s > hardcoded in qemuDomainGetBlkioParameters. > >> >> "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. > > Hmm, makes sense. Actually, it’s turned out there is/was a problem… virTypedParamsFilter validates that params must be NON-NULL (IMHO, params == NULL is valid if nparams == 0, but as you said, it’s not safe, since all other code paths might assume other things). Therefore, it is best to leave it as it is for nparams == 0. But then of course we have to change it in the bootstrap.conf to calloc-gnu (as suggested by Daniel)! However, the rest of the patch should still be valid (the assignment of *remote_params_len only at the end). If you want to I can resend an appropriate version. Note: The same applies to the second patch. Thank you. > >> >> 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. > > Okay. > >> >> John >> > -- > 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
