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. > > 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
