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) \@@ -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