Re: [Libguestfs] [PATCH] drives: fix deletion of servers on error
On Monday 18 August 2014 17:30:27 Richard W.M. Jones wrote:
On Mon, Aug 18, 2014 at 02:56:08PM +0200, Pino Toscano wrote:
> Make sure to not skip any of the created server, and to always free
> the "server" array.
> diff --git a/src/drives.c b/src/drives.c
> index 4bd8328..85c1495 100644
> --- a/src/drives.c
> +++ b/src/drives.c
> @@ -743,8 +743,7 @@ parse_servers (guestfs_h *g, char *const *strs,
>
> for (i = 0; i < n; ++i) {
>
> if (parse_one_server (g, strs[i], &servers[i]) == -1) {
>
> - if (i > 0)
> - free_drive_servers (servers, i-1);
> + free_drive_servers (servers, i);
>
> return -1;
>
> }
>
> }
>
> ---
>
> src/drives.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/src/drives.c b/src/drives.c
> index 4bd8328..85c1495 100644
> --- a/src/drives.c
> +++ b/src/drives.c
> @@ -743,8 +743,7 @@ parse_servers (guestfs_h *g, char *const *strs,
>
> for (i = 0; i < n; ++i) {
>
> if (parse_one_server (g, strs[i], &servers[i]) == -1) {
>
> - if (i > 0)
> - free_drive_servers (servers, i-1);
> + free_drive_servers (servers, i);
>
> return -1;
>
> }
>
> }
The original code is attempting to avoid a double-free of
servers[i].u.hostname. I think this change would mean the double-free
would happen again.
It shouldn't.
I still don't understand the motivation for this change.
Basically in case of errors when parsing a server, we are leaking
possibly one or two things, depending on the situation.
Taking the original code:
for (i = 0; i < n; ++i) {
if (parse_one_server (g, strs[i], &servers[i]) == -1) {
if (i > 0)
free_drive_servers (servers, i-1);
return -1;
}
}
1) if the first element (i == 0) cannot be parsed, then the "servers"
array is leaked; for example with:
guestfish add-drive /disk server:"invalid_host"
2) if the faulty element is second and beyond, then there will be i
elements before the faulty i-th, while the code frees i-1 elements
(leaking the (i-1)-th one); for example with:
guestfish add-drive /disk server:"tcp:example.com invalid_host"
--
Pino Toscano