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

[Marc Hartmayer]

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.

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/

>
> 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