On Wednesday 21 January 2015 10:39:12 Hu Tao wrote:

On Tue, Jan 20, 2015 at 04:28:40PM +0100, Pino Toscano wrote:

...

The code is already within a "if (prompt) {...}" block, so checking for "prompt" again is redundant. --- fish/fish.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fish/fish.c b/fish/fish.c index 8b74c7b..71db83a 100644 --- a/fish/fish.c +++ b/fish/fish.c @@ -661,8 +661,8 @@ rl_gets (int prompt) line_read = NULL; }

- p = prompt && ps1 ? decode_ps1 (ps1) : NULL; - line_read = readline (prompt ? (ps1 ? p : FISH) : ""); + p = ps1 ? decode_ps1 (ps1) : NULL; + line_read = readline (ps1 ? p : FISH);

Can be simplified further:

line_read = readline (ps1 ? decode_ps1 (ps1) : FISH);

decode_ps1 returns a new char* buffer, hence it must be freed (notice that its return value is assigned to a CLEANUP_FREE variable). -- Pino Toscano

On Tue, Jan 20, 2015 at 04:28:41PM +0100, Pino Toscano wrote:

Make sure to free the backend string after using it. --- src/launch-libvirt.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/launch-libvirt.c b/src/launch-libvirt.c index 899742f..6f95e98 100644 --- a/src/launch-libvirt.c +++ b/src/launch-libvirt.c @@ -719,6 +719,8 @@ parse_capabilities (guestfs_h *g, const char *capabilities_xml, * appliance VM. */ if (!seen_qemu && !seen_kvm) { + CLEANUP_FREE char *backend = guestfs_get_backend (g); + error (g, _("libvirt hypervisor doesn't support qemu or KVM,\n" "so we cannot create the libguestfs appliance.\n" @@ -730,7 +732,7 @@ parse_capabilities (guestfs_h *g, const char *capabilities_xml, "Or: if you want to have libguestfs run qemu directly, try:\n" " export LIBGUESTFS_BACKEND=direct\n" "For further help, read the guestfs(3) man page and libguestfs FAQ."), - guestfs_get_backend (g)); + backend); return -1; }

-- 1.9.3

ACK. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org

On Wednesday 21 January 2015 10:28:33 Hu Tao wrote:

On Tue, Jan 20, 2015 at 04:28:39PM +0100, Pino Toscano wrote:

...

If "strdup (d->d_name)" fails with "i" > 0, then both "p" and "ret->guestfs_int_dirent_list_val" are non-null pointers, but the latter is no more valid (since "p" is the new realloc'ed buffer). Hence, trying to free both will access to invalid memory.

Make sure to free only one of them, "p" if not null or "ret->guestfs_int_dirent_list_val" otherwise. --- daemon/readdir.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/daemon/readdir.c b/daemon/readdir.c index f0ddd21..e488f93 100644 --- a/daemon/readdir.c +++ b/daemon/readdir.c @@ -27,6 +27,17 @@ #include "daemon.h" #include "actions.h"

+static void +free_int_dirent_list (guestfs_int_dirent *p, size_t len) +{ + size_t i; + + for (i = 0; i < len; ++i) { + free (p[i].name); + } + free (p); +} + guestfs_int_dirent_list * do_readdir (const char *path) { @@ -64,8 +75,11 @@ do_readdir (const char *path) v.name = strdup (d->d_name); if (!p || !v.name) { reply_with_perror ("allocate"); - free (ret->guestfs_int_dirent_list_val); - free (p); + if (p) { + free_int_dirent_list (p, i);

I think free() is the correct way to free memory here, since the memory is allocated by realloc().

free_int_dirent_list in the end calls free on the whole array, but each element has a char* member which need to be freed too. -- Pino Toscano