10:12 a.m.
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