Re: [Libvirt-cim] [PATCH 2/5] libxkutil: Support for device addresses

On 11/12/2013 11:34 PM, John Ferlan wrote: [...] >> +int add_device_address_property(struct device_address *devaddr, >> + const char *key, >> + const char *value) >> +{ >> + char *k = NULL; >> + char *v = NULL; >> + char **list = NULL; >> + >> + if (key != NULL && value != NULL) { >> + k = strdup(key); >> + v = strdup(value); >> + if (k == NULL || v == NULL) >> + goto err; >> + >> + list = realloc(devaddr->key, sizeof(char*) * (devaddr->ct+1)); >> + if (list == NULL) >> + goto err; >> + devaddr->key = list; >> + >> + list = realloc(devaddr->value, sizeof(char*) * (devaddr->ct+1)); >> + if (list == NULL) > > There's a leak here since 'ct' isn't incremented until later, > devaddr->key will have the 'list' from above and that'll either be lost > or overwritten. I have to admit that you're not the first reviewer I have confused with this approach ... so: If realloc of the 'value' list fails, the devaddr content remains valid and unchanged, exactly because we don't increment the counter. And the intent is to preserve the passed-in devaddr even if adding new key-value pairs fails. It's true that the 'key' list will have an excess element, which doesn't hurt as it doesn't need to be freed (it has not been set). Frankly, I don't have a better idea, unless not to use realloc but do malloc's followed by memcpy's instead.