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

[Richard W.M. Jones]

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