Libguestfs - Jul 2014 - [PATCH 0/2] supermin: improve handling of memory

Hi,

the two patches improve the way memory is handled in supermin, by
cleanly exiting on memory allocation failures, and free'ing memory when
not needed (to keep working and not run out of memory).


Pino Toscano (2):
  Check for failures in memory allocations
  Free memory buffers when not used

 src/ext2fs-c.c | 13 +++++++++++--
 src/init.c     | 13 +++++++++++++
 2 files changed, 24 insertions(+), 2 deletions(-)

-- 
1.9.3

Make sure to report the failure (usually exiting) in case of memory
allocation failures.
---
 src/ext2fs-c.c |  4 ++++
 src/init.c     | 13 +++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/src/ext2fs-c.c b/src/ext2fs-c.c
index a265ad9..99f4d3c 100644
--- a/src/ext2fs-c.c
+++ b/src/ext2fs-c.c
@@ -615,6 +615,10 @@ ext2_copy_file (ext2_filsys fs, const char *src, const char
*dest)
       if (fp == NULL)
 	goto cont;
       new_dirname = malloc (PATH_MAX+1);
+      if (new_dirname == NULL) {
+	pclose (fp);
+	goto cont;
+      }
       if (fgets (new_dirname, PATH_MAX, fp) == NULL) {
 	pclose (fp);
 	goto cont;
diff --git a/src/init.c b/src/init.c
index 73efdff..6b3dc7e 100644
--- a/src/init.c
+++ b/src/init.c
@@ -307,6 +307,11 @@ insmod (const char *filename)
   errno = 0;
   size = 0;
 
+  if (!buf) {
+    perror("malloc");
+    exit (EXIT_FAILURE);
+  }
+
 #ifdef HAVE_LZMA_STATIC
   if (ends_with(filename, ".xz")) {
     lzma_stream strm = LZMA_STREAM_INIT;
@@ -350,6 +355,10 @@ insmod (const char *filename)
           const size_t num =  tmpsize - strm.avail_out;
           if (num > capacity) {
                buf = (char*) realloc (buf, size*2);
+               if (!buf) {
+                    perror("realloc");
+                    exit (EXIT_FAILURE);
+               }
                capacity = size;
           }
           memcpy (buf+size, tmp_out, num);
@@ -380,6 +389,10 @@ insmod (const char *filename)
   while ((num = gzread (gzfp, tmp, tmpsize)) > 0) {
     if (num > capacity) {
       buf = (char*) realloc (buf, size*2);
+      if (!buf) {
+        perror("realloc");
+        exit (EXIT_FAILURE);
+      }
       capacity = size;
     }
     memcpy (buf+size, tmp, num);
-- 
1.9.3

While supermin is a short-lived application, some of the allocated
buffers depend on sizes from the filesystem being read, which could lead
to keep too much memory allocated and potentially not allowing supermin
to run properly.
---
 src/ext2fs-c.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/src/ext2fs-c.c b/src/ext2fs-c.c
index 99f4d3c..70755c9 100644
--- a/src/ext2fs-c.c
+++ b/src/ext2fs-c.c
@@ -511,6 +511,7 @@ read_whole_file (const char *filename, size_t size)
   if (fd == -1) {
     /* Skip unreadable files. */
     fprintf (stderr, "supermin: open: %s: %m\n", filename);
+    free (buf);
     return NULL;
   }
 
@@ -585,11 +586,12 @@ ext2_copy_file (ext2_filsys fs, const char *src, const
char *dest)
    * is because the code was copied and pasted from supermin 4 where
    * there was no such distinction.
    */
-  const char *dirname, *basename;
+  char *dirname;
+  const char *basename;
   const char *p = strrchr (dest, '/');
   ext2_ino_t dir_ino;
   if (dest == p) {     /* "/foo" */
-    dirname = "/";
+    dirname = strdup ("/");
     basename = dest+1;
     dir_ino = EXT2_ROOT_INO;
   } else {                      /* "/foo/bar" */
@@ -630,6 +632,7 @@ ext2_copy_file (ext2_filsys fs, const char *src, const char
*dest)
 	  new_dirname[len-1] == '\n')
 	new_dirname[len-1] = '\0';
 
+      free (dirname);
       dirname = new_dirname;
     }
   cont:
@@ -717,4 +720,6 @@ ext2_copy_file (ext2_filsys fs, const char *src, const char
*dest)
                       major (statbuf.st_rdev), minor (statbuf.st_rdev),
                       dir_ft, NULL);
   }
+
+  free (dirname);
 }
-- 
1.9.3

On Tue, Jul 29, 2014 at 06:47:40PM +0200, Pino Toscano
wrote:
> Make sure to report the failure (usually exiting) in case of memory
> allocation failures.
> ---
>  src/ext2fs-c.c |  4 ++++
>  src/init.c     | 13 +++++++++++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/src/ext2fs-c.c b/src/ext2fs-c.c
> index a265ad9..99f4d3c 100644
> --- a/src/ext2fs-c.c
> +++ b/src/ext2fs-c.c
> @@ -615,6 +615,10 @@ ext2_copy_file (ext2_filsys fs, const char *src, const
char *dest)
>        if (fp == NULL)
>  	goto cont;
>        new_dirname = malloc (PATH_MAX+1);
> +      if (new_dirname == NULL) {
> +	pclose (fp);
> +	goto cont;
> +      }
>        if (fgets (new_dirname, PATH_MAX, fp) == NULL) {
>  	pclose (fp);
>  	goto cont;
> diff --git a/src/init.c b/src/init.c
> index 73efdff..6b3dc7e 100644
> --- a/src/init.c
> +++ b/src/init.c
> @@ -307,6 +307,11 @@ insmod (const char *filename)
>    errno = 0;
>    size = 0;
>  
> +  if (!buf) {
> +    perror("malloc");
> +    exit (EXIT_FAILURE);
> +  }
> +
>  #ifdef HAVE_LZMA_STATIC
>    if (ends_with(filename, ".xz")) {
>      lzma_stream strm = LZMA_STREAM_INIT;
> @@ -350,6 +355,10 @@ insmod (const char *filename)
>            const size_t num =  tmpsize - strm.avail_out;
>            if (num > capacity) {
>                 buf = (char*) realloc (buf, size*2);
> +               if (!buf) {
> +                    perror("realloc");
> +                    exit (EXIT_FAILURE);
> +               }
>                 capacity = size;
>            }
>            memcpy (buf+size, tmp_out, num);
> @@ -380,6 +389,10 @@ insmod (const char *filename)
>    while ((num = gzread (gzfp, tmp, tmpsize)) > 0) {
>      if (num > capacity) {
>        buf = (char*) realloc (buf, size*2);
> +      if (!buf) {
> +        perror("realloc");
> +        exit (EXIT_FAILURE);
> +      }
>        capacity = size;
>      }
>      memcpy (buf+size, tmp, num);
ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

On Tue, Jul 29, 2014 at 06:47:41PM +0200, Pino Toscano
wrote:
> While supermin is a short-lived application, some of the allocated
> buffers depend on sizes from the filesystem being read, which could lead
> to keep too much memory allocated and potentially not allowing supermin
> to run properly.
ACK.

See also:

https://bugzilla.redhat.com/show_bug.cgi?id=1099172

I've only worked around this (by excluding various /var/log files).

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW