Pino Toscano
2015-Jan-20 15:28 UTC
[Libguestfs] [PATCH] daemon: readdir: fix invalid memory access on error
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);
+ } else {
+ free_int_dirent_list (ret->guestfs_int_dirent_list_val, i);
+ }
free (v.name);
free (ret);
closedir (dir);
--
1.9.3
Pino Toscano
2015-Jan-20 15:28 UTC
[Libguestfs] [PATCH] fish: remove extra "prompt" checks
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);
if (ps_output) { /* GUESTFISH_OUTPUT */
CLEANUP_FREE char *po = decode_ps1 (ps_output);
--
1.9.3
Pino Toscano
2015-Jan-20 15:28 UTC
[Libguestfs] [PATCH] launch: libvirt: do not leak the backend string on error
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
Hu Tao
2015-Jan-21 02:28 UTC
Re: [Libguestfs] [PATCH] daemon: readdir: fix invalid memory access on error
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(). Regards, Hu> + } else { > + free_int_dirent_list (ret->guestfs_int_dirent_list_val, i); > + } > free (v.name); > free (ret); > closedir (dir); > -- > 1.9.3 > > _______________________________________________ > Libguestfs mailing list > Libguestfs@redhat.com > https://www.redhat.com/mailman/listinfo/libguestfs
Hu Tao
2015-Jan-21 02:30 UTC
Re: [Libguestfs] [PATCH] launch: libvirt: do not leak the backend string on error
Reviewed-by: Hu Tao <hutao@cn.fujitsu.com> 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 > > _______________________________________________ > Libguestfs mailing list > Libguestfs@redhat.com > https://www.redhat.com/mailman/listinfo/libguestfs
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); Regards, Hu> > if (ps_output) { /* GUESTFISH_OUTPUT */ > CLEANUP_FREE char *po = decode_ps1 (ps_output); > -- > 1.9.3 > > _______________________________________________ > Libguestfs mailing list > Libguestfs@redhat.com > https://www.redhat.com/mailman/listinfo/libguestfs
Pino Toscano
2015-Jan-21 09:25 UTC
Re: [Libguestfs] [PATCH] daemon: readdir: fix invalid memory access on error
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
Richard W.M. Jones
2015-Jan-21 14:00 UTC
Re: [Libguestfs] [PATCH] daemon: readdir: fix invalid memory access on error
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); > + } else { > + free_int_dirent_list (ret->guestfs_int_dirent_list_val, i); > + } > free (v.name); > free (ret); > closedir (dir); > -- > 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
Richard W.M. Jones
2015-Jan-21 14:00 UTC
Re: [Libguestfs] [PATCH] fish: remove extra "prompt" checks
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); > > if (ps_output) { /* GUESTFISH_OUTPUT */ > CLEANUP_FREE char *po = decode_ps1 (ps_output); > -- > 1.9.3ACK. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
Richard W.M. Jones
2015-Jan-21 14:01 UTC
Re: [Libguestfs] [PATCH] launch: libvirt: do not leak the backend string on error
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.3ACK. 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