[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [fedora-virt] [PATCH] add comments suggesting memory-handling improvements



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 redhat com>
>> 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 redhat com>
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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]