I'm not someone who thinks VLAs are automatically bad and unlike Linux kernel code they can sometimes be used safely in userspace. However for an internet exposed server there is an argument that they might cause some kind of exploitable situation especially if the code is compiled without other stack hardening features. Also in highly multithreaded code with limited stack sizes (as nbdkit is on some platforms) allowing unbounded stack allocation can be a bad idea because it can cause a segfault. So this commit bans them. Only when using --enable-gcc-warnings, but upstream developers ought to be using that. There were in fact only two places where VLAs were being used. In one of those places (plugins/sh) removing the VLA actually made the code better. For interesting discussion about VLAs in the kernel see: https://lwn.net/Articles/763253/ --- configure.ac | 2 +- plugins/sh/sh.c | 7 +++---- server/sockets.c | 8 +++++++- 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/configure.ac b/configure.ac index 5842c6f..d326377 100644 --- a/configure.ac +++ b/configure.ac @@ -90,7 +90,7 @@ AC_ARG_ENABLE([gcc-warnings], [gcc_warnings=no] ) if test "x$gcc_warnings" = "xyes"; then - WARNINGS_CFLAGS="-Wall -Wshadow -Werror" + WARNINGS_CFLAGS="-Wall -Wshadow -Wvla -Werror" AC_SUBST([WARNINGS_CFLAGS]) fi diff --git a/plugins/sh/sh.c b/plugins/sh/sh.c index c73b08b..acb50c4 100644 --- a/plugins/sh/sh.c +++ b/plugins/sh/sh.c @@ -74,8 +74,7 @@ sh_load (void) static void sh_unload (void) { - const size_t tmpdir_len = strlen (tmpdir); - char cmd[7 + tmpdir_len + 1]; /* "rm -rf " + tmpdir + \0 */ + CLEANUP_FREE char *cmd = NULL; /* Run the unload method. Ignore all errors. */ if (script) { @@ -85,8 +84,8 @@ sh_unload (void) } /* Delete the temporary directory. Ignore all errors. */ - snprintf (cmd, sizeof cmd, "rm -rf %s", tmpdir); - system (cmd); + if (asprintf (&cmd, "rm -rf %s", tmpdir) >= 0) + system (cmd); free (script); } diff --git a/server/sockets.c b/server/sockets.c index 26d65c6..dfaa3ea 100644 --- a/server/sockets.c +++ b/server/sockets.c @@ -366,10 +366,16 @@ accept_connection (int listen_sock) static void check_sockets_and_quit_fd (int *socks, size_t nr_socks) { - struct pollfd fds[nr_socks + 1]; size_t i; int r; + CLEANUP_FREE struct pollfd *fds = + malloc (sizeof (struct pollfd) * (nr_socks+1)); + if (fds == NULL) { + perror ("malloc"); + exit (EXIT_FAILURE); + } + for (i = 0; i < nr_socks; ++i) { fds[i].fd = socks[i]; fds[i].events = POLLIN; -- 2.23.0