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

[John Ferlan]

On 06/07/2018 08:17 AM, Marc Hartmayer wrote:
> On Wed, May 09, 2018 at 09:51 PM +0200, John Ferlan <jferlan at redhat.com> wrote:
>> On 05/07/2018 11:24 AM, Marc Hartmayer wrote:
>>> On Mon, Apr 30, 2018 at 03:20 PM +0200, John Ferlan <jferlan at redhat.com> 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, 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
>