Hi Noah,
On Thu, 2021-07-22 at 15:22 -0400, Noah Sanci via Elfutils-devel wrote:
> PR27983 improvements attached
Please do mention the improvements you made. It really does help others
see what you thought of. So in this case use asprintf instead of calloc
plus sprintf, and (re)use tmpurl instead of strduping it and freeing.
Both good improvements.
I really like this variant of the patch. Nice work.
There is one possible issue with the reallocarray call (realloc[array]
is terrible that way, sorry). See below.
Also, could you please rebase this patch on top of master, there have
been various changes to debuginfod-client.c and it would be nice to see
how the patch interacts with those.
> + else
> + {
> + num_urls++;
> + server_url_list = reallocarray(server_url_list, num_urls,
> + sizeof(char*));
> + if (server_url_list == NULL)
> + {
> + rc = -ENOMEM;
> + goto out1;
> + }
> + server_url_list[num_urls-1] = tmp_url;
> + }
Note how reallocarray returns NULL on failure.
This means you lost the original server_url_list pointer.
And so when you then do cleanup by going to out1...
> + out1:
> + for (int i = 0; i < num_urls; ++i)
> + free(server_url_list[i]);
> + free(server_url_list);
You will crash because server_url_list is NULL here, so
server_url_list[i] is a bad dereference...
Bleah. Like I said, the realloc(array) interface is really bad :{
To use reallocarray safely you need to use a temp and only set it after
the call succeeds.
char **realloc_ptr = reallocarray (...);
if (realloc_ptr == NULL)
{
rc = -ENOMEM;
goto out1;
}
server_url_list = realloc_ptr;
server_url_list[num_urls-1] = ...;
The rest of the patch looks good.
Thanks,
Mark
