On 9/5/19 6:28 AM, Richard W.M. Jones wrote: > 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(-)
> +++ 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" I'm guessing that both gcc and clang are okay with our current list; we may reach the point where we need to probe at configure time on which options we can safely use, instead of merely open-coding a list, but we'll deal with that when it breaks the build. > +++ 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); Safe only because our tmpdir pattern contains no shell metacharacters (your patch does not change that fact). If we ever decided to honor $TMPDIR (that is, creating $TMPDIR/nbdkitshXXXXXX instead of /tmp/nbdkitshXXXXXX), then we'd need shell quoting here. But doesn't change this patch. > +++ 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)); This is indeed safer, but adds a malloc() in a loop. Thankfully, the loop of accept_incoming_connections() doesn't cycle that quickly (once per new client), so I think it's in the noise compared to pre-allocating the array once before starting the loop. ACK. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs