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

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



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


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