Libguestfs - Jan 2015 - Re: [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

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

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

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

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

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

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

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.3
ACK.

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/

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