[Libguestfs] [PATCH nbdkit] Ban use of stack Variable Length Arrays (VLAs).

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20190905/44e8a969/attachment.sig>