[fedora-virt] [PATCH] add comments suggesting memory-handling improvements
Jim Meyering
jim at meyering.net
Wed Jul 1 16:21:05 UTC 2009
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 at 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 at 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
More information about the Fedora-virt
mailing list