thr3ads.net - Libguestfs - [Libguestfs] [PATCH] daemon: readdir: fix invalid memory access on error [Jan 2015]

If this information is useful, please help other people find it:
Share via:

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

head link

[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

head link

[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

head link

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

head link

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

Hu Tao

2015-Jan-21 02:39 UTC

head link

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);
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

head link

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

head link

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

head link

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.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/

Richard W.M. Jones

2015-Jan-21 14:01 UTC

head link

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

Apparently Analagous Threads

Search for more reasonably related threads

Libguestfs - Jan 2015 - [PATCH] daemon: readdir: fix invalid memory access on error

[Libguestfs] [PATCH] daemon: readdir: fix invalid memory access on error

[Libguestfs] [PATCH] fish: remove extra "prompt" checks

[Libguestfs] [PATCH] launch: libvirt: do not leak the backend string on error

Re: [Libguestfs] [PATCH] daemon: readdir: fix invalid memory access on error

Re: [Libguestfs] [PATCH] launch: libvirt: do not leak the backend string on error

Re: [Libguestfs] [PATCH] fish: remove extra "prompt" checks

Re: [Libguestfs] [PATCH] daemon: readdir: fix invalid memory access on error

Re: [Libguestfs] [PATCH] daemon: readdir: fix invalid memory access on error

Re: [Libguestfs] [PATCH] fish: remove extra "prompt" checks

Re: [Libguestfs] [PATCH] launch: libvirt: do not leak the backend string on error

Apparently Analagous Threads