[fedora-virt] [PATCH] add comments suggesting memory-handling improvements
Richard W.M. Jones
rjones at redhat.com
Wed Jul 1 16:38:05 UTC 2009
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 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.
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
More information about the Fedora-virt
mailing list