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