[PATCH] add comments suggesting memory-handling improvements

[Jim Meyering]

Hi Rich,

Looking at the sole remaining uses of asprintf, I noticed these
and some realloc/strdup results that needed to be checked.

Re asprintf, note that one must always check it for failure,
and when it fails, you cannot use the (undefined) pointer result.
Obviously this patch is just for reference, i.e, not to apply.

> From 7b7df29516dad78a3c815ea1ef301da89d4b5323 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering(a)redhat.com&gt;
Date: Wed, 1 Jul 2009 16:09:33 +0200
Subject: [PATCH] add comments suggesting memory-handling improvements

* fish/destpaths.c: Don't deref NULL or undef on failed heap alloc.
---
 fish/destpaths.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/fish/destpaths.c b/fish/destpaths.c
index 6cddafa..a5ece2a 100644
--- a/fish/destpaths.c
+++ b/fish/destpaths.c
@@ -55,7 +55,7 @@ complete_dest_paths_generator (const char *text, int state)

   static int len, index;
   static char **words = NULL;
-  static int nr_words = 0;
+  static int nr_words = 0; // nr_words should be size_t
   char *word;
   guestfs_error_handler_cb old_error_cb;
   void *old_error_cb_data;
@@ -75,6 +75,7 @@ complete_dest_paths_generator (const char *text, int state)
     char **strs;
     int i, n;

+    // len should be of type size_t; index and "i", too
     len = strlen (text);
     index = 0;

@@ -92,8 +93,11 @@ complete_dest_paths_generator (const char *text, int state)

 #define APPEND_STRS_AND_FREE						\
     if (strs) {								\
+      // n should be unsigned, like size_t
       n = count_strings (strs);						\
+      // handle the case in which sizeof (char *) * (nr_words + n)) overflows
       words = realloc (words, sizeof (char *) * (nr_words + n));	\
+      // handle failed realloc
       for (i = 0; i < n; ++i)						\
 	words[nr_words++] = strs[i];					\
       free (strs);							\
@@ -120,17 +124,20 @@ complete_dest_paths_generator (const char *text, int state)

       p = strrchr (text, '/');
       dir = p && p > text ? strndup (text, p - text) : strdup
("/");
+      // handle failed strdup

       strs = guestfs_ls (g, dir);

       /* Prepend directory to names. */
       if (strs) {
 	for (i = 0; strs[i]; ++i) {
+	  int err;
 	  p = NULL;
 	  if (strcmp (dir, "/") == 0)
-	    asprintf (&p, "/%s", strs[i]);
+	    err = asprintf (&p, "/%s", strs[i]);
 	  else
-	    asprintf (&p, "%s/%s", dir, strs[i]);
+	    err = asprintf (&p, "%s/%s", dir, strs[i]);
+	  // handle failed asprintf, i.e., err < 0
 	  free (strs[i]);
 	  strs[i] = p;
 	}
--
1.6.3.3.483.g4f5e

[Richard W.M. Jones]

On Wed, Jul 01, 2009 at 04:11:50PM +0200, Jim Meyering wrote:
>  Hi Rich,
>  
>  Looking at the sole remaining uses of asprintf, I noticed these
>  and some realloc/strdup results that needed to be checked.
>  
>  Re asprintf, note that one must always check it for failure,
>  and when it fails, you cannot use the (undefined) pointer result.
>  Obviously this patch is just for reference, i.e, not to apply.
>  
>  >From 7b7df29516dad78a3c815ea1ef301da89d4b5323 Mon Sep 17 00:00:00 2001
>  From: Jim Meyering <meyering(a)redhat.com&gt;
>  Date: Wed, 1 Jul 2009 16:09:33 +0200
>  Subject: [PATCH] add comments suggesting memory-handling improvements
>  
>  * fish/destpaths.c: Don't deref NULL or undef on failed heap alloc.
>  ---
>   fish/destpaths.c |   13 ++++++++++---
>   1 files changed, 10 insertions(+), 3 deletions(-)
>  
>  diff --git a/fish/destpaths.c b/fish/destpaths.c
>  index 6cddafa..a5ece2a 100644
>  --- a/fish/destpaths.c
>  +++ b/fish/destpaths.c
>  @@ -55,7 +55,7 @@ complete_dest_paths_generator (const char *text, int state)
>  
>     static int len, index;
>     static char **words = NULL;
>  -  static int nr_words = 0;
>  +  static int nr_words = 0; // nr_words should be size_t
>     char *word;
>     guestfs_error_handler_cb old_error_cb;
>     void *old_error_cb_data;
>  @@ -75,6 +75,7 @@ complete_dest_paths_generator (const char *text, int state)
>       char **strs;
>       int i, n;
>  
>  +    // len should be of type size_t; index and "i", too
>       len = strlen (text);
>       index = 0;
>  
>  @@ -92,8 +93,11 @@ complete_dest_paths_generator (const char *text, int state)
>  
>   #define APPEND_STRS_AND_FREE						\
>       if (strs) {								\
>  +      // n should be unsigned, like size_t
>         n = count_strings (strs);						\
>  +      // handle the case in which sizeof (char *) * (nr_words + n)) overflows
>         words = realloc (words, sizeof (char *) * (nr_words + n));	\
>  +      // handle failed realloc
>         for (i = 0; i < n; ++i)						\
>   	words[nr_words++] = strs[i];					\
>         free (strs);							\
>  @@ -120,17 +124,20 @@ complete_dest_paths_generator (const char *text, int state)
>  
>         p = strrchr (text, '/');
>         dir = p && p > text ? strndup (text, p - text) : strdup
> ("/");
>  +      // handle failed strdup 
[etc]

Was the plan to add these comments to the source now in the hope
we'd fix it in future?

Rich.

-- 
Richard Jones, Emerging Technologies, Red Hat  http://et.redhat.com/~rjones
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://et.redhat.com/~rjones/virt-top

[Jim Meyering]

Richard W.M. Jones wrote:
>  On Wed, Jul 01, 2009 at 04:11:50PM +0200, Jim Meyering wrote:
> > Re asprintf, note that one must always check it for failure,
> > and when it fails, you cannot use the (undefined) pointer result.
> > Obviously this patch is just for reference, i.e, not to apply. 
                                                    ^^^^^^^^^^^^^
> > >From 7b7df29516dad78a3c815ea1ef301da89d4b5323 Mon Sep 17
> 00:00:00 2001
> > From: Jim Meyering <meyering(a)redhat.com&gt;
> > Date: Wed, 1 Jul 2009 16:09:33 +0200
> > Subject: [PATCH] add comments suggesting memory-handling improvements 
...
> >  #define APPEND_STRS_AND_FREE						\
> >      if (strs) {								\
> > +      // n should be unsigned, like size_t
> >        n = count_strings (strs);						\
> > +      // handle the case in which sizeof (char *) * (nr_words + n)) overflows
> >        words = realloc (words, sizeof (char *) * (nr_words + n));	\
> > +      // handle failed realloc
> >        for (i = 0; i < n; ++i)						\
> >  	words[nr_words++] = strs[i];					\
> >        free (strs);							\
> > @@ -120,17 +124,20 @@ complete_dest_paths_generator (const char *text, int state)
> >
> >        p = strrchr (text, '/');
> >        dir = p && p > text ? strndup (text, p - text) : strdup
> ("/");
> > +      // handle failed strdup
>  [etc]
>
>  Was the plan to add these comments to the source now in the hope
>  we'd fix it in future? 
Please, don't apply that ;-)  It's not even syntactically correct.

I've include a patch below.  It would have been easier and cleaner *looking*
to use functions like xrealloc, xstrdup, etc. that exit upon failure here.
But since nothing else seems to exit from this code, I bit the bullet:
(this compiles, but "make check" hasn't completed yet)
Hmm... new test failures on F11 (details below)
But I'd just pulled latest and rebased, too.

> From 77b4a54834cd3d3e6994508104334f501f5d99f1 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering(a)redhat.com&gt;
Date: Wed, 1 Jul 2009 16:09:33 +0200
Subject: [PATCH] fish: handle some out-of-memory conditions

* fish/destpaths.c (xalloc_oversized): Define.
(complete_dest_paths_generator): Use size_t as type for a few
variables, rather than int.
Don't deref NULL or undef on failed heap alloc.
Don't leak on failed realloc.
Detect theoretical overflow when count_strings returns a very
large number of strings.
Handle asprintf failure.
(APPEND_STRS_AND_FREE): Rewrite as do {...}while(0), so that each use
can/must be followed by a semicolon.  Better for auto-formatters.
---
 fish/destpaths.c |   91 ++++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 62 insertions(+), 29 deletions(-)

diff --git a/fish/destpaths.c b/fish/destpaths.c
index 6cddafa..90970de 100644
--- a/fish/destpaths.c
+++ b/fish/destpaths.c
@@ -1,5 +1,5 @@
 /* guestfish - the filesystem interactive shell
- * Copyright (C) 2009 Red Hat Inc.
+ * Copyright (C) 2009 Red Hat Inc.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -22,6 +22,7 @@

 #include <stdio.h>
 #include <stdlib.h>
+#include <stddef.h>
 #include <string.h>

 #ifdef HAVE_LIBREADLINE
@@ -32,6 +33,22 @@

 #include "fish.h"

+// From gnulib's xalloc.h:
+/* Return 1 if an array of N objects, each of size S, cannot exist due
+   to size arithmetic overflow.  S must be positive and N must be
+   nonnegative.  This is a macro, not an inline function, so that it
+   works correctly even when SIZE_MAX < N.
+
+   By gnulib convention, SIZE_MAX represents overflow in size
+   calculations, so the conservative dividend to use here is
+   SIZE_MAX - 1, since SIZE_MAX might represent an overflowed value.
+   However, malloc (SIZE_MAX) fails on all known hosts where
+   sizeof (ptrdiff_t) <= sizeof (size_t), so do not bother to test for
+   exactly-SIZE_MAX allocations on such hosts; this avoids a test and
+   branch when S is known to be 1.  */
+# define xalloc_oversized(n, s) \
+    ((size_t) (sizeof (ptrdiff_t) <= sizeof (size_t) ? -1 : -2) / (s) < (n))
+
 /* Readline completion for paths on the guest filesystem, also for
  * devices and LVM names.
  */
@@ -53,9 +70,9 @@ complete_dest_paths_generator (const char *text, int state)
 {
 #ifdef HAVE_LIBREADLINE

-  static int len, index;
+  static size_t len, index;
   static char **words = NULL;
-  static int nr_words = 0;
+  static size_t nr_words = 0;
   char *word;
   guestfs_error_handler_cb old_error_cb;
   void *old_error_cb_data;
@@ -73,12 +90,12 @@ complete_dest_paths_generator (const char *text, int state)

   if (!state) {
     char **strs;
-    int i, n;

     len = strlen (text);
     index = 0;

     if (words) {
+      size_t i;
       /* NB. 'words' array is NOT NULL-terminated. */
       for (i = 0; i < nr_words; ++i)
 	free (words[i]);
@@ -90,26 +107,38 @@ complete_dest_paths_generator (const char *text, int state)

     SAVE_ERROR_CB

+/* Silently do nothing if an allocation fails */
 #define APPEND_STRS_AND_FREE						\
+  do {									\
     if (strs) {								\
-      n = count_strings (strs);						\
-      words = realloc (words, sizeof (char *) * (nr_words + n));	\
-      for (i = 0; i < n; ++i)						\
-	words[nr_words++] = strs[i];					\
-      free (strs);							\
-    }
+      size_t n = count_strings (strs);					\
+      if ( ! xalloc_oversized (nr_words + n, sizeof (char *))) {	\
+	char *w = realloc (words, sizeof (char *) * (nr_words + n));	\
+	if (w == NULL) {						\
+	  free (words);							\
+	  words = NULL;							\
+	  nr_words = 0;							\
+	} else {							\
+	  size_t i;							\
+	  for (i = 0; i < n; ++i)					\
+	    words[nr_words++] = strs[i];				\
+	}								\
+	free (strs);							\
+      }									\
+    }									\
+  } while (0)

     /* Is it a device? */
     if (len < 5 || strncmp (text, "/dev/", 5) == 0) {
       /* Get a list of everything that can possibly begin with /dev/ */
       strs = guestfs_list_devices (g);
-      APPEND_STRS_AND_FREE
+      APPEND_STRS_AND_FREE;

       strs = guestfs_list_partitions (g);
-      APPEND_STRS_AND_FREE
+      APPEND_STRS_AND_FREE;

       strs = guestfs_lvs (g);
-      APPEND_STRS_AND_FREE
+      APPEND_STRS_AND_FREE;
     }

     if (len < 1 || text[0] == '/') {
@@ -120,24 +149,28 @@ complete_dest_paths_generator (const char *text, int state)

       p = strrchr (text, '/');
       dir = p && p > text ? strndup (text, p - text) : strdup
("/");
-
-      strs = guestfs_ls (g, dir);
-
-      /* Prepend directory to names. */
-      if (strs) {
-	for (i = 0; strs[i]; ++i) {
-	  p = NULL;
-	  if (strcmp (dir, "/") == 0)
-	    asprintf (&p, "/%s", strs[i]);
-	  else
-	    asprintf (&p, "%s/%s", dir, strs[i]);
-	  free (strs[i]);
-	  strs[i] = p;
+      if (dir) {
+	strs = guestfs_ls (g, dir);
+
+	/* Prepend directory to names. */
+	if (strs) {
+	  size_t i;
+	  for (i = 0; strs[i]; ++i) {
+	    int err;
+	    if (strcmp (dir, "/") == 0)
+	      err = asprintf (&p, "/%s", strs[i]);
+	    else
+	      err = asprintf (&p, "%s/%s", dir, strs[i]);
+	    if (0 <= err) {
+	      free (strs[i]);
+	      strs[i] = p;
+	    }
+	  }
 	}
-      }

-      free (dir);
-      APPEND_STRS_AND_FREE
+	free (dir);
+	APPEND_STRS_AND_FREE;
+      }
     }

     /* else ...  In theory we could complete other things here such as VG
--
1.6.3.3.483.g4f5e

 16/174 test_head_n_1
mount: /dev/vda1 on /: mount: wrong fs type, bad option, bad superblock on /dev/vda1,
       missing codepage or helper program, or other error
       In some cases useful info is found in syslog - try
       dmesg | tail  or so
test_head_n_1 FAILED
...
 25/174 test_glob_expand_0
mount: /dev/vda1 on /: mount: unknown filesystem type 'lvm2pv'
test_glob_expand_0 FAILED
 26/174 test_glob_expand_1
/sbin/sfdisk /dev/vda: external command failed
test_glob_expand_1 FAILED
 27/174 test_glob_expand_2
/sbin/sfdisk /dev/vda: external command failed
test_glob_expand_2 FAILED
 28/174 test_ntfs_3g_probe_0
/sbin/sfdisk /dev/vda: external command failed
test_ntfs_3g_probe_0 FAILED
 29/174 test_ntfs_3g_probe_1
/sbin/sfdisk /dev/vda: external command failed
test_ntfs_3g_probe_1 FAILED
...
 32/174 test_find_1
mount: /dev/vda1 on /: mount: wrong fs type, bad option, bad superblock on /dev/vda1,
       missing codepage or helper program, or other error
       In some cases useful info is found in syslog - try
       dmesg | tail  or so
test_find_1 FAILED
 33/174 test_find_2
 34/174 test_lvresize_0
 35/174 test_zerofree_0
vgremove: VG: File descriptor 3 (socket:[5132]) leaked on lvm invocation. Parent PID 1:
  /dev/dm-0: stat failed: No such file or directory
  Path /dev/dm-0 no longer valid for device(253,0)
  /dev/block/253:0: stat failed: No such file or directory
  Pa
test_zerofree_0 FAILED
 36/174 test_hexdump_0
open: /new: Stale NFS file handle
test_hexdump_0 FAILED
 37/174 test_hexdump_1
 38/174 test_strings_e_0
open: /new: Stale NFS file handle
test_strings_e_0 FAILED
 39/174 test_strings_e_1
        test_strings_e_1 skipped (reason: test disabled in generator)
 40/174 test_strings_0
...
 52/174 test_cp_1
mount: /dev/vda1 on /: mount: Stale NFS file handle
test_cp_1 FAILED

[Richard W.M. Jones]

On Wed, Jul 01, 2009 at 06:21:05PM +0200, Jim Meyering wrote:
>   16/174 test_head_n_1
>  mount: /dev/vda1 on /: mount: wrong fs type, bad option, bad superblock on /dev/vda1,
>         missing codepage or helper program, or other error
>         In some cases useful info is found in syslog - try
>         dmesg | tail  or so
>  test_head_n_1 FAILED
>  ...
>   25/174 test_glob_expand_0
>  mount: /dev/vda1 on /: mount: unknown filesystem type 'lvm2pv'
>  test_glob_expand_0 FAILED 
Yes, sorry, I just broke the test suite.  Fix coming up
as soon as I know it passes 'make check' ...

Rich.

-- 
Richard Jones, Emerging Technologies, Red Hat  http://et.redhat.com/~rjones
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://et.redhat.com/~rjones/virt-top

[Richard W.M. Jones]

On Wed, Jul 01, 2009 at 05:32:24PM +0100, Richard W.M. Jones wrote:
>  On Wed, Jul 01, 2009 at 06:21:05PM +0200, Jim Meyering wrote:
>  >  16/174 test_head_n_1
>  > mount: /dev/vda1 on /: mount: wrong fs type, bad option, bad superblock on
> /dev/vda1,
>  >        missing codepage or helper program, or other error
>  >        In some cases useful info is found in syslog - try
>  >        dmesg | tail  or so
>  > test_head_n_1 FAILED
>  > ...
>  >  25/174 test_glob_expand_0
>  > mount: /dev/vda1 on /: mount: unknown filesystem type 'lvm2pv'
>  > test_glob_expand_0 FAILED
>  
>  Yes, sorry, I just broke the test suite.  Fix coming up
>  as soon as I know it passes 'make check' ... 
Do you want to try it again now, with the latest from git.

Specifically,

http://git.et.redhat.com/?p=libguestfs.git;a=commitdiff;h=fe27753ae5925cb...
http://git.et.redhat.com/?p=libguestfs.git;a=commitdiff;h=3e2d925717d1dac...
http://git.et.redhat.com/?p=libguestfs.git;a=commitdiff;h=99e28249d52ca54...

Rich.

-- 
Richard Jones, Emerging Technologies, Red Hat  http://et.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into Xen guests.
http://et.redhat.com/~rjones/virt-p2v

[Jim Meyering]

Richard W.M. Jones wrote:
>  On Wed, Jul 01, 2009 at 05:32:24PM +0100, Richard W.M. Jones wrote:
> > On Wed, Jul 01, 2009 at 06:21:05PM +0200, Jim Meyering wrote:
> > >  16/174 test_head_n_1
> > > mount: /dev/vda1 on /: mount: wrong fs type, bad option, bad superblock on
> /dev/vda1,
> > >        missing codepage or helper program, or other error
> > >        In some cases useful info is found in syslog - try
> > >        dmesg | tail  or so
> > > test_head_n_1 FAILED
> > > ...
> > >  25/174 test_glob_expand_0
> > > mount: /dev/vda1 on /: mount: unknown filesystem type 'lvm2pv'
> > > test_glob_expand_0 FAILED
> >
> > Yes, sorry, I just broke the test suite.  Fix coming up
> > as soon as I know it passes 'make check' ...
>
>  Do you want to try it again now, with the latest from git. 
Rebased and running the tests...
31/174 and counting.

[Jim Meyering]

Jim Meyering wrote:

>  Richard W.M. Jones wrote:
> > On Wed, Jul 01, 2009 at 05:32:24PM +0100, Richard W.M. Jones wrote:
> >> On Wed, Jul 01, 2009 at 06:21:05PM +0200, Jim Meyering wrote:
> >> >  16/174 test_head_n_1
> >> > mount: /dev/vda1 on /: mount: wrong fs type, bad option, bad superblock on
> /dev/vda1,
> >> >        missing codepage or helper program, or other error
> >> >        In some cases useful info is found in syslog - try
> >> >        dmesg | tail  or so
> >> > test_head_n_1 FAILED
> >> > ...
> >> >  25/174 test_glob_expand_0
> >> > mount: /dev/vda1 on /: mount: unknown filesystem type 'lvm2pv'
> >> > test_glob_expand_0 FAILED
> >>
> >> Yes, sorry, I just broke the test suite.  Fix coming up
> >> as soon as I know it passes 'make check' ...
> >
> > Do you want to try it again now, with the latest from git.
>
>  Rebased and running the tests...
>  31/174 and counting. 
All tests passed, so I pushed.

[Richard W.M. Jones]

On Wed, Jul 01, 2009 at 06:21:05PM +0200, Jim Meyering wrote:
>  I've include a patch below.  It would have been easier and
> cleaner *looking*
>  to use functions like xrealloc, xstrdup, etc. that exit upon failure here.
>  But since nothing else seems to exit from this code, I bit the bullet:
>  (this compiles, but "make check" hasn't completed yet) 
Yes, I agree.  There's not very much that guestfish can do if it runs
out of memory, and the most sensible thing would be just to exit with
an error (or abort).

>  Hmm... new test failures on F11 (details below)
>  But I'd just pulled latest and rebased, too.
>  
>  >From 77b4a54834cd3d3e6994508104334f501f5d99f1 Mon Sep 17 00:00:00 2001
>  From: Jim Meyering <meyering(a)redhat.com&gt;
>  Date: Wed, 1 Jul 2009 16:09:33 +0200
>  Subject: [PATCH] fish: handle some out-of-memory conditions
>  
>  * fish/destpaths.c (xalloc_oversized): Define.
>  (complete_dest_paths_generator): Use size_t as type for a few
>  variables, rather than int.
>  Don't deref NULL or undef on failed heap alloc.
>  Don't leak on failed realloc.
>  Detect theoretical overflow when count_strings returns a very
>  large number of strings.
>  Handle asprintf failure.
>  (APPEND_STRS_AND_FREE): Rewrite as do {...}while(0), so that each use
>  can/must be followed by a semicolon.  Better for auto-formatters.
>  ---
>   fish/destpaths.c |   91 ++++++++++++++++++++++++++++++++++++-----------------
>   1 files changed, 62 insertions(+), 29 deletions(-)
>  
>  diff --git a/fish/destpaths.c b/fish/destpaths.c
>  index 6cddafa..90970de 100644
>  --- a/fish/destpaths.c
>  +++ b/fish/destpaths.c
>  @@ -1,5 +1,5 @@
>   /* guestfish - the filesystem interactive shell
>  - * Copyright (C) 2009 Red Hat Inc.
>  + * Copyright (C) 2009 Red Hat Inc. 
Strange, this doesn't look like any change?

>    * This program is free software; you can redistribute it and/or
> modify
>    * it under the terms of the GNU General Public License as published by
>  @@ -22,6 +22,7 @@
>  
>   #include <stdio.h>
>   #include <stdlib.h>
>  +#include <stddef.h>
>   #include <string.h>
>  
>   #ifdef HAVE_LIBREADLINE
>  @@ -32,6 +33,22 @@
>  
>   #include "fish.h"
>  
>  +// From gnulib's xalloc.h:
>  +/* Return 1 if an array of N objects, each of size S, cannot exist due
>  +   to size arithmetic overflow.  S must be positive and N must be
>  +   nonnegative.  This is a macro, not an inline function, so that it
>  +   works correctly even when SIZE_MAX < N.
>  +
>  +   By gnulib convention, SIZE_MAX represents overflow in size
>  +   calculations, so the conservative dividend to use here is
>  +   SIZE_MAX - 1, since SIZE_MAX might represent an overflowed value.
>  +   However, malloc (SIZE_MAX) fails on all known hosts where
>  +   sizeof (ptrdiff_t) <= sizeof (size_t), so do not bother to test for
>  +   exactly-SIZE_MAX allocations on such hosts; this avoids a test and
>  +   branch when S is known to be 1.  */
>  +# define xalloc_oversized(n, s) \
>  +    ((size_t) (sizeof (ptrdiff_t) <= sizeof (size_t) ? -1 : -2) / (s) < (n))
>  +
>   /* Readline completion for paths on the guest filesystem, also for
>    * devices and LVM names.
>    */
>  @@ -53,9 +70,9 @@ complete_dest_paths_generator (const char *text, int state)
>   {
>   #ifdef HAVE_LIBREADLINE
>  
>  -  static int len, index;
>  +  static size_t len, index;
>     static char **words = NULL;
>  -  static int nr_words = 0;
>  +  static size_t nr_words = 0;
>     char *word;
>     guestfs_error_handler_cb old_error_cb;
>     void *old_error_cb_data;
>  @@ -73,12 +90,12 @@ complete_dest_paths_generator (const char *text, int state)
>  
>     if (!state) {
>       char **strs;
>  -    int i, n;
>  
>       len = strlen (text);
>       index = 0;
>  
>       if (words) {
>  +      size_t i;
>         /* NB. 'words' array is NOT NULL-terminated. */
>         for (i = 0; i < nr_words; ++i)
>   	free (words[i]);
>  @@ -90,26 +107,38 @@ complete_dest_paths_generator (const char *text, int state)
>  
>       SAVE_ERROR_CB
>  
>  +/* Silently do nothing if an allocation fails */
>   #define APPEND_STRS_AND_FREE						\
>  +  do {									\
>       if (strs) {								\
>  -      n = count_strings (strs);						\
>  -      words = realloc (words, sizeof (char *) * (nr_words + n));	\
>  -      for (i = 0; i < n; ++i)						\
>  -	words[nr_words++] = strs[i];					\
>  -      free (strs);							\
>  -    }
>  +      size_t n = count_strings (strs);					\
>  +      if ( ! xalloc_oversized (nr_words + n, sizeof (char *))) {	\
>  +	char *w = realloc (words, sizeof (char *) * (nr_words + n));	\
>  +	if (w == NULL) {						\
>  +	  free (words);							\
>  +	  words = NULL;							\
>  +	  nr_words = 0;							\
>  +	} else {							\
>  +	  size_t i;							\
>  +	  for (i = 0; i < n; ++i)					\
>  +	    words[nr_words++] = strs[i];				\
>  +	}								\
>  +	free (strs);							\
>  +      }									\
>  +    }									\
>  +  } while (0)
>  
>       /* Is it a device? */
>       if (len < 5 || strncmp (text, "/dev/", 5) == 0) {
>         /* Get a list of everything that can possibly begin with /dev/ */
>         strs = guestfs_list_devices (g);
>  -      APPEND_STRS_AND_FREE
>  +      APPEND_STRS_AND_FREE;
>  
>         strs = guestfs_list_partitions (g);
>  -      APPEND_STRS_AND_FREE
>  +      APPEND_STRS_AND_FREE;
>  
>         strs = guestfs_lvs (g);
>  -      APPEND_STRS_AND_FREE
>  +      APPEND_STRS_AND_FREE;
>       } 
Yes, this all makes sense.

>       if (len < 1 || text[0] == '/') {
>  @@ -120,24 +149,28 @@ complete_dest_paths_generator (const char *text, int state)
>  
>         p = strrchr (text, '/');
>         dir = p && p > text ? strndup (text, p - text) : strdup
> ("/");
>  -
>  -      strs = guestfs_ls (g, dir);
>  -
>  -      /* Prepend directory to names. */
>  -      if (strs) {
>  -	for (i = 0; strs[i]; ++i) {
>  -	  p = NULL;
>  -	  if (strcmp (dir, "/") == 0)
>  -	    asprintf (&p, "/%s", strs[i]);
>  -	  else
>  -	    asprintf (&p, "%s/%s", dir, strs[i]);
>  -	  free (strs[i]);
>  -	  strs[i] = p;
>  +      if (dir) {
>  +	strs = guestfs_ls (g, dir);
>  +
>  +	/* Prepend directory to names. */
>  +	if (strs) {
>  +	  size_t i;
>  +	  for (i = 0; strs[i]; ++i) {
>  +	    int err;
>  +	    if (strcmp (dir, "/") == 0)
>  +	      err = asprintf (&p, "/%s", strs[i]);
>  +	    else
>  +	      err = asprintf (&p, "%s/%s", dir, strs[i]);
>  +	    if (0 <= err) {
>  +	      free (strs[i]);
>  +	      strs[i] = p;
>  +	    }
>  +	  }
>   	}
>  -      }
>  
>  -      free (dir);
>  -      APPEND_STRS_AND_FREE
>  +	free (dir);
>  +	APPEND_STRS_AND_FREE;
>  +      }
>       } 
Yes, all looks good.

ACK.

Rich.

-- 
Richard Jones, Emerging Technologies, Red Hat  http://et.redhat.com/~rjones
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://et.redhat.com/~rjones/virt-top

[Jim Meyering]

Richard W.M. Jones wrote:

>  On Wed, Jul 01, 2009 at 06:21:05PM +0200, Jim Meyering wrote:
> > I've include a patch below.  It would have been easier and cleaner *looking*
> > to use functions like xrealloc, xstrdup, etc. that exit upon failure here.
> > But since nothing else seems to exit from this code, I bit the bullet:
> > (this compiles, but "make check" hasn't completed yet)
>
>  Yes, I agree.  There's not very much that guestfish can do if it runs
>  out of memory, and the most sensible thing would be just to exit with
>  an error (or abort).
>
> > Hmm... new test failures on F11 (details below)
> > But I'd just pulled latest and rebased, too.
> >
> > >From 77b4a54834cd3d3e6994508104334f501f5d99f1 Mon Sep 17 00:00:00 2001
> > From: Jim Meyering <meyering(a)redhat.com&gt;
> > Date: Wed, 1 Jul 2009 16:09:33 +0200
> > Subject: [PATCH] fish: handle some out-of-memory conditions
> >
> > * fish/destpaths.c (xalloc_oversized): Define.
> > (complete_dest_paths_generator): Use size_t as type for a few
> > variables, rather than int.
> > Don't deref NULL or undef on failed heap alloc.
> > Don't leak on failed realloc.
> > Detect theoretical overflow when count_strings returns a very
> > large number of strings.
> > Handle asprintf failure.
> > (APPEND_STRS_AND_FREE): Rewrite as do {...}while(0), so that each use
> > can/must be followed by a semicolon.  Better for auto-formatters.
> > ---
> >  fish/destpaths.c |   91 ++++++++++++++++++++++++++++++++++++-----------------
> >  1 files changed, 62 insertions(+), 29 deletions(-)
> >
> > diff --git a/fish/destpaths.c b/fish/destpaths.c
> > index 6cddafa..90970de 100644
> > --- a/fish/destpaths.c
> > +++ b/fish/destpaths.c
> > @@ -1,5 +1,5 @@
> >  /* guestfish - the filesystem interactive shell
> > - * Copyright (C) 2009 Red Hat Inc.
> > + * Copyright (C) 2009 Red Hat Inc.
>
>  Strange, this doesn't look like any change? 
As I sent it, I accidentally let a write hook remove the trailing blank
on the "- ..." line.

I.e., that change (before being munged) removes the sole
trailing blank in that file.