[libvirt] [PATCH 0/2] Minor fixes for virTypedParams(De)Serialize
Some minor fixes and two questions: * Is the first method, which is described in the documentation for virTypedParamsDeserialize, in sync with the actual code? ("Older APIs do not rely on deserializer allocating memory for @params, ...") * Do we also have to set *nparams = 0 in case of an error and the user has allocated the memory? (virTypedParamsDeserialize) Marc Hartmayer (2): virTypedParamsSerialize: minor fixes virTypedParamsDeserialize: minor fixes src/util/virtypedparam.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) -- 2.13.4
1. Don't allocate if there is nothing that needs to be allocated. Especially as the result of calling calloc(0, ...) is implementation-defined. 2. Update the length @remote_params_len only if the related @remote_params_val has also been set. Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/util/virtypedparam.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index 2452628cdbcd..10fd28baa65c 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -1500,10 +1500,10 @@ virTypedParamsSerialize(virTypedParameterPtr params, size_t i; size_t j; int rv = -1; - virTypedParameterRemotePtr params_val; + virTypedParameterRemotePtr params_val = NULL; + int params_len = nparams; - *remote_params_len = nparams; - if (VIR_ALLOC_N(params_val, nparams) < 0) + if (nparams && VIR_ALLOC_N(params_val, nparams) < 0) goto cleanup; for (i = 0, j = 0; i < nparams; ++i) { @@ -1515,7 +1515,7 @@ virTypedParamsSerialize(virTypedParameterPtr params, if (!param->type || (!(flags & VIR_TYPED_PARAM_STRING_OKAY) && param->type == VIR_TYPED_PARAM_STRING)) { - --*remote_params_len; + --params_len; continue; } @@ -1556,6 +1556,7 @@ virTypedParamsSerialize(virTypedParameterPtr params, } *remote_params_val = params_val; + *remote_params_len = params_len; params_val = NULL; rv = 0; -- 2.13.4
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.
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 thus the question becomes is there a more specific path you are referencing here? 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. 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. John
Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/util/virtypedparam.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index 2452628cdbcd..10fd28baa65c 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -1500,10 +1500,10 @@ virTypedParamsSerialize(virTypedParameterPtr params, size_t i; size_t j; int rv = -1; - virTypedParameterRemotePtr params_val; + virTypedParameterRemotePtr params_val = NULL; + int params_len = nparams;
- *remote_params_len = nparams; - if (VIR_ALLOC_N(params_val, nparams) < 0) + if (nparams && VIR_ALLOC_N(params_val, nparams) < 0) goto cleanup;
for (i = 0, j = 0; i < nparams; ++i) { @@ -1515,7 +1515,7 @@ virTypedParamsSerialize(virTypedParameterPtr params, if (!param->type || (!(flags & VIR_TYPED_PARAM_STRING_OKAY) && param->type == VIR_TYPED_PARAM_STRING)) { - --*remote_params_len; + --params_len; continue; }
@@ -1556,6 +1556,7 @@ virTypedParamsSerialize(virTypedParameterPtr params, }
*remote_params_val = params_val; + *remote_params_len = params_len; params_val = NULL; rv = 0;
On Mon, Apr 30, 2018 at 09:20:29AM -0400, John Ferlan 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.
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 thus the question becomes is there a more specific path you are referencing here?
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. 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.
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. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, Apr 30, 2018 at 03:20 PM +0200, John Ferlan <jferlan@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.
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).
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.
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
On 05/07/2018 11:24 AM, Marc Hartmayer wrote:
On Mon, Apr 30, 2018 at 03:20 PM +0200, John Ferlan <jferlan@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(¶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? 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. 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
On Wed, May 09, 2018 at 09:51 PM +0200, John Ferlan <jferlan@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@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(¶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. 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
On 06/07/2018 08:17 AM, Marc Hartmayer wrote:
On Wed, May 09, 2018 at 09:51 PM +0200, John Ferlan <jferlan@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@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(¶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, 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]
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
On Wed, Jun 13, 2018 at 03:11 PM +0200, John Ferlan <jferlan@redhat.com> wrote:
On 06/07/2018 08:17 AM, Marc Hartmayer wrote:
On Wed, May 09, 2018 at 09:51 PM +0200, John Ferlan <jferlan@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@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(¶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]
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, ¶ms, &nparams); assert(params == NULL && assert nparams == 0); Do we? […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
On Thu, Jun 21, 2018 at 03:47 PM +0200, Marc Hartmayer <mhartmay@linux.ibm.com> wrote:
On Wed, Jun 13, 2018 at 03:11 PM +0200, John Ferlan <jferlan@redhat.com> wrote:
On 06/07/2018 08:17 AM, Marc Hartmayer wrote:
On Wed, May 09, 2018 at 09:51 PM +0200, John Ferlan <jferlan@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@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(¶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.
Maybe it was an empty dictionary. […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
On Thu, Jun 21, 2018 at 03:47 PM +0200, Marc Hartmayer <mhartmay@linux.ibm.com> wrote:
On Wed, Jun 13, 2018 at 03:11 PM +0200, John Ferlan <jferlan@redhat.com> wrote:
On 06/07/2018 08:17 AM, Marc Hartmayer wrote:
On Wed, May 09, 2018 at 09:51 PM +0200, John Ferlan <jferlan@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@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(¶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]
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] -- 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
On 07/03/2018 06:32 AM, Marc Hartmayer wrote:
On Thu, Jun 21, 2018 at 03:47 PM +0200, Marc Hartmayer <mhartmay@linux.ibm.com> wrote:
On Wed, Jun 13, 2018 at 03:11 PM +0200, John Ferlan <jferlan@redhat.com> wrote:
On 06/07/2018 08:17 AM, Marc Hartmayer wrote:
On Wed, May 09, 2018 at 09:51 PM +0200, John Ferlan <jferlan@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@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(¶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]
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. :-). 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
On Tue, Jul 03, 2018 at 09:14 PM +0200, John Ferlan <jferlan@redhat.com> wrote:
On 07/03/2018 06:32 AM, Marc Hartmayer wrote:
On Thu, Jun 21, 2018 at 03:47 PM +0200, Marc Hartmayer <mhartmay@linux.ibm.com> wrote:
On Wed, Jun 13, 2018 at 03:11 PM +0200, John Ferlan <jferlan@redhat.com> wrote:
On 06/07/2018 08:17 AM, Marc Hartmayer wrote:
On Wed, May 09, 2018 at 09:51 PM +0200, John Ferlan <jferlan@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@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(¶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]
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.
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
On Wed, Jul 04, 2018 at 11:24 AM +0200, Marc Hartmayer <mhartmay@linux.ibm.com> wrote:
On Tue, Jul 03, 2018 at 09:14 PM +0200, John Ferlan <jferlan@redhat.com> wrote:
On 07/03/2018 06:32 AM, Marc Hartmayer wrote:
On Thu, Jun 21, 2018 at 03:47 PM +0200, Marc Hartmayer <mhartmay@linux.ibm.com> wrote:
On Wed, Jun 13, 2018 at 03:11 PM +0200, John Ferlan <jferlan@redhat.com> wrote:
On 06/07/2018 08:17 AM, Marc Hartmayer wrote:
On Wed, May 09, 2018 at 09:51 PM +0200, John Ferlan <jferlan@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@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(¶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]
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
On 07/06/2018 05:11 AM, Marc Hartmayer wrote:
On Wed, Jul 04, 2018 at 11:24 AM +0200, Marc Hartmayer <mhartmay@linux.ibm.com> wrote:
On Tue, Jul 03, 2018 at 09:14 PM +0200, John Ferlan <jferlan@redhat.com> wrote:
On 07/03/2018 06:32 AM, Marc Hartmayer wrote:
On Thu, Jun 21, 2018 at 03:47 PM +0200, Marc Hartmayer <mhartmay@linux.ibm.com> wrote:
On Wed, Jun 13, 2018 at 03:11 PM +0200, John Ferlan <jferlan@redhat.com> wrote:
On 06/07/2018 08:17 AM, Marc Hartmayer wrote: > On Wed, May 09, 2018 at 09:51 PM +0200, John Ferlan <jferlan@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@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(¶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?
Perhaps callback is the wrong term here - still the relationship between Serialize and Deserialize and expectations or long standing assumptions of @nparams/@params is of great concern. Chasing each possible caller is time consuming.
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.
Well, good question. The virTypedParamsDeserialize function comments give me enough pause to wonder. When I looked at a couple of paths there were checks that nparams == 0 prior to even getting to the Serializer call (using cscope egrep on "nparams == 0" in various remote_*.c callers, but there are a couple that get all the way to the driver before returning some constant value (qemu_driver does this for BlockIoTune and BlockStats and others for GetCPUStats). I didn't dig beyond that.
"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.
I think best to send something that's updated and go from there. Hopefully we can get more engagement from others on the team. John
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
1. Don't allocate if there is nothing that needs to be allocated. Especially as the result of calling calloc(0, ...) is implementation-defined. 2. Reset @nparams to 0 in case of an error. Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/util/virtypedparam.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index 10fd28baa65c..01716b517ad2 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -1402,7 +1402,7 @@ virTypedParamsDeserialize(virTypedParameterRemotePtr remote_params, goto cleanup; } } else { - if (VIR_ALLOC_N(*params, remote_params_len) < 0) + if (remote_params_len && VIR_ALLOC_N(*params, remote_params_len) < 0) goto cleanup; } *nparams = remote_params_len; @@ -1467,6 +1467,7 @@ virTypedParamsDeserialize(virTypedParameterRemotePtr remote_params, } else { virTypedParamsFree(*params, i); *params = NULL; + *nparams = 0; } } return rv; -- 2.13.4
participants (4)
-
Daniel P. Berrangé -
John Ferlan -
Marc Hartmayer -
Marc Hartmayer