[Libguestfs] [PATCH v2] Use less stack.
Pino Toscano
ptoscano at redhat.com
Mon Mar 7 16:12:33 UTC 2016
On Monday 07 March 2016 11:18:32 Richard W.M. Jones wrote:
> GCC has two warnings related to large stack frames. We were already
> using the -Wframe-larger-than warning, but this reduces the threshold
> from 10000 to 5000 bytes.
>
> However that warning only covers the static part of frames (not
> alloca). So this change also enables -Wstack-usage=10000 which covers
> both the static and dynamic usage (alloca and variable length arrays).
>
> Multiple changes are made throughout the code to reduce frames to fit
> within these new limits.
>
> Note that stack allocation of large strings can be a security issue.
> For example, we had code like:
>
> size_t len = strlen (fs->windows_systemroot) + 64;
> char software[len];
> snprintf (software, len, "%s/system32/config/software",
> fs->windows_systemroot);
>
> where fs->windows_systemroot is guest controlled. It's not clear what
> the effects might be of allowing the guest to allocate potentially
> very large stack frames, but at best it allows the guest to cause
> libguestfs to segfault. It turns out we are very lucky that
> fs->windows_systemroot cannot be set arbitrarily large (see checks in
> is_systemroot).
>
> This commit changes those to large heap allocations instead.
The general idea and changes of this patch is good, although there are
few places to fix.
> --- a/daemon/btrfs.c
> +++ b/daemon/btrfs.c
> @@ -94,6 +94,11 @@ btrfs_set_label (const char *device, const char *label)
> return 0;
> }
>
> +#if defined(__GNUC__) && GUESTFS_GCC_VERSION >= 40800 /* gcc >= 4.8.0 */
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Wstack-usage=10000"
> +#endif
> +
> /* Takes optional arguments, consult optargs_bitmask. */
> int
> do_btrfs_filesystem_resize (const char *filesystem, int64_t size)
Is this because of the argv[MAX_ARGS] here ^ and in do_mkfs_btrfs?
(Similar notes for all the other diagnostic markers added by this
patch.)
As slightly related change (although that can be done after this patch),
something to reduce a bit the stacks in the daemon would be reduce the
MAX_ARGS to values closer (but not too much) than the actual needs.
I see most of the MAX_ARGS=64 cases have no more than 10 arguments
added, even in the worst case...
> diff --git a/daemon/dd.c b/daemon/dd.c
> index d29c527..461df61 100644
> --- a/daemon/dd.c
> +++ b/daemon/dd.c
> @@ -106,15 +106,21 @@ do_copy_size (const char *src, const char *dest, int64_t ssize)
> }
>
> uint64_t position = 0, size = (uint64_t) ssize;
> + CLEANUP_FREE char *buf;
Missing "= NULL" here.
> diff --git a/daemon/debug.c b/daemon/debug.c
> index 5637939..6059603 100644
> --- a/daemon/debug.c
> +++ b/daemon/debug.c
> @@ -437,12 +437,18 @@ static char *
> debug_ls (const char *subcmd, size_t argc, char *const *const argv)
> {
> size_t len = count_strings (argv);
> - const char *cargv[len+3];
> + CLEANUP_FREE const char **cargv;
Missing "= NULL" here.
> diff --git a/daemon/fill.c b/daemon/fill.c
> index a9a3aa7..b1d2180 100644
> --- a/daemon/fill.c
> +++ b/daemon/fill.c
> @@ -35,12 +35,18 @@ do_fill (int c, int len, const char *path)
> ssize_t r;
> size_t len_sz;
> size_t n;
> - char buf[BUFSIZ];
> + CLEANUP_FREE char *buf;
Missing "= NULL" here.
> @@ -127,13 +133,16 @@ do_fill_pattern (const char *pattern, int len, const char *path)
> int
> do_fill_dir (const char *dir, int n)
> {
> - size_t len = strlen (dir);
> - char filename[len+10];
> int fd;
> int i;
>
> for (i = 0; i < n; ++i) {
> - snprintf (filename, len+10, "%s/%08d", dir, i);
> + CLEANUP_FREE char *filename;
Missing "= NULL" here.
> diff --git a/daemon/luks.c b/daemon/luks.c
> index a720e25..b8530c5 100644
> --- a/daemon/luks.c
> +++ b/daemon/luks.c
> @@ -91,9 +91,11 @@ luks_open (const char *device, const char *key, const char *mapname,
> * that the device-mapper control device (/dev/mapper/control) is
> * always there, so you can't ever have mapname == "control".
> */
> - size_t len = strlen (mapname);
> - char devmapper[len+32];
> - snprintf (devmapper, len+32, "/dev/mapper/%s", mapname);
> + CLEANUP_FREE char *devmapper;
Missing "= NULL" here.
> diff --git a/daemon/upload.c b/daemon/upload.c
> index efbb427..bb6da39 100644
> --- a/daemon/upload.c
> +++ b/daemon/upload.c
> @@ -152,7 +152,13 @@ int
> do_download (const char *filename)
> {
> int fd, r, is_dev;
> - char buf[GUESTFS_MAX_CHUNK_SIZE];
> + CLEANUP_FREE char *buf;
Missing "= NULL" here.
> diff --git a/erlang/erl-guestfs-proto.c b/erlang/erl-guestfs-proto.c
> index 658a0ef..0c2f545 100644
> --- a/erlang/erl-guestfs-proto.c
> +++ b/erlang/erl-guestfs-proto.c
> @@ -201,11 +201,14 @@ ETERM *
> make_string_list (char **r)
> {
> size_t i, size;
> + ETERM **t;
>
> for (size = 0; r[size] != NULL; ++size)
> ;
>
> - ETERM *t[size];
> + t = malloc (sizeof (ETERM *) * size);
> + if (t == NULL)
> + return make_error ("make_string_list");
't' is leaked now.
>
> for (i = 0; r[i] != NULL; ++i)
> t[i] = erl_mk_string (r[i]);
> @@ -220,13 +223,16 @@ ETERM *
> make_table (char **r)
> {
> size_t i, size;
> -
> - for (size = 0; r[size] != NULL; ++size)
> - ;
> -
> - ETERM *t[size/2];
> + ETERM **t;
> ETERM *a[2];
>
> + for (size = 0; r[size] != NULL; ++size)
> + ;
> +
> + t = malloc (sizeof (ETERM *) * (size/2));
> + if (t == NULL)
> + return make_error ("make_table");
Ditto.
> diff --git a/examples/mount-local.c b/examples/mount-local.c
> index 291cb26..8bd6401 100644
> --- a/examples/mount-local.c
> +++ b/examples/mount-local.c
> @@ -142,8 +142,13 @@ main (int argc, char *argv[])
> p = strrchr (shell, '/');
> if (p && strcmp (p+1, "bash") == 0) {
> size_t len = 64 + strlen (shell);
> - char buf[len];
> + char *buf;
>
> + buf = malloc (len);
> + if (buf == NULL) {
> + perror ("malloc");
> + _exit (EXIT_FAILURE);
> + }
> snprintf (buf, len, "PS1='mount-local-shell> ' %s --norc -i", shell);
> r = system (buf);
While this is just an example, making it not leak will make it a better
example :)
> diff --git a/fish/glob.c b/fish/glob.c
> index 9fba42e..01b7c6d 100644
> --- a/fish/glob.c
> +++ b/fish/glob.c
> @@ -285,13 +296,19 @@ single_element_list (const char *element)
> return pp;
> }
>
> -static void
> +static int
> glob_issue (char *cmd, size_t argc,
> char ***globs, size_t *posn, size_t *count,
> int *r)
> {
> size_t i;
> - char *argv[argc+1];
> + CLEANUP_FREE char **argv;
Missing "= NULL" here.
> diff --git a/fish/hexedit.c b/fish/hexedit.c
> index 9f051aa..b8c0a91 100644
> --- a/fish/hexedit.c
> +++ b/fish/hexedit.c
> @@ -100,7 +100,8 @@ run_hexedit (const char *cmd, size_t argc, char *argv[])
> const char *editor;
> int r;
> struct stat oldstat, newstat;
> - char buf[BUFSIZ];
> + CLEANUP_FREE char *tmpfd = NULL;
> + CLEANUP_FREE char *editcmd = NULL;
>
> CLEANUP_FREE char *tmpdir = guestfs_get_tmpdir (g), *tmp = NULL;
> if (asprintf (&tmp, "%s/guestfishXXXXXX", tmpdir) == -1) {
> @@ -119,9 +120,12 @@ run_hexedit (const char *cmd, size_t argc, char *argv[])
> if (editor == NULL)
> editor = "hexedit";
>
> - snprintf (buf, sizeof buf, "/dev/fd/%d", fd);
> + if (asprintf (&tmpfd, "/dev/fd/%d", fd) == -1) {
IMHO this could stay as stack buffer: usually 3*sizeof(type) is used to
indicate the maximum number of characters needed to represent some
integer type, so the size of buf is known at build time:
char buf[sizeof ("/dev/fd/%d") + sizeof(int)*3];
It will be slightly bigger than needed, but still short enough.
> diff --git a/p2v/conversion.c b/p2v/conversion.c
> index f5c518a..8e90ef5 100644
> --- a/p2v/conversion.c
> +++ b/p2v/conversion.c
> @@ -167,7 +167,7 @@ start_conversion (struct config *config,
> int status;
> size_t i, len;
> size_t nr_disks = guestfs_int_count_strings (config->disks);
> - struct data_conn data_conns[nr_disks];
> + CLEANUP_FREE struct data_conn *data_conns;
Missing "= NULL" here.
> diff --git a/src/appliance.c b/src/appliance.c
> index dbde35e..d7cc60b 100644
> --- a/src/appliance.c
> +++ b/src/appliance.c
> @@ -396,27 +386,24 @@ find_path (guestfs_h *g,
>
> /* Returns true iff file is contained in dir. */
> static int
> -dir_contains_file (const char *dir, const char *file)
> +dir_contains_file (guestfs_h *g, const char *dir, const char *file)
> {
> - size_t dirlen = strlen (dir);
> - size_t filelen = strlen (file);
> - size_t len = dirlen + filelen + 2;
> - char path[len];
> + CLEANUP_FREE char *path;
Missing "= NULL" here.
Thanks,
--
Pino Toscano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20160307/92093cab/attachment.sig>
More information about the Libguestfs
mailing list