From f20854ec61eef1aea313920f0cf193a78c1a9219 Mon Sep 17 00:00:00 2001 From: Jim Meyering Date: Wed, 1 Jul 2009 16:09:33 +0200 Subject: [PATCH 1/1] 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 file 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 #include +#include #include #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.8.3.1