Re: [PATCH] glob: Add new test tst-glob-tilde

[Carlos O'Donell <carlos at redhat dot com>]

On 10/20/2017 04:24 AM, Florian Weimer wrote:
> The new test checks for memory leaks (see bug 22325) and attempts
> to trigger the buffer overflow in bug 22320.
> 
> 2017-10-20  Florian Weimer  <fweimer@redhat.com>
> 
> 	* posix/Makefile (tests): Add tst-glob-tilde.
> 	(tests-special): Add tst-glob-tilde-mem.out
> 	(tst-glob-tilde-ENV): Set MALLOC_TRACE.
> 	(tst-glob-tilde-mem.out): Add mtrace check.
> 	* posix/tst-glob-tilde.c: New file.

OK with an added comment in the test loop.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> diff --git a/posix/Makefile b/posix/Makefile
> index b5894425ae..d4a5299a52 100644
> --- a/posix/Makefile
> +++ b/posix/Makefile
> @@ -94,7 +94,8 @@ tests		:= test-errno tstgetopt testfnm runtests runptests \
>  		   tst-fnmatch3 bug-regex36 tst-getaddrinfo5 \
>  		   tst-posix_spawn-fd tst-posix_spawn-setsid \
>  		   tst-posix_fadvise tst-posix_fadvise64 \
> -		   tst-sysconf-empty-chroot tst-glob_symlinks tst-fexecve
> +		   tst-sysconf-empty-chroot tst-glob_symlinks tst-fexecve \
> +		   tst-glob-tilde

OK.

>  tests-internal	:= bug-regex5 bug-regex20 bug-regex33 \
>  		   tst-rfc3484 tst-rfc3484-2 tst-rfc3484-3 \
>  		   tst-glob_lstat_compat
> @@ -143,7 +144,8 @@ tests-special += $(objpfx)bug-regex2-mem.out $(objpfx)bug-regex14-mem.out \
>  		 $(objpfx)tst-rxspencer-no-utf8-mem.out $(objpfx)tst-pcre-mem.out \
>  		 $(objpfx)tst-boost-mem.out $(objpfx)tst-getconf.out \
>  		 $(objpfx)bug-glob2-mem.out $(objpfx)tst-vfork3-mem.out \
> -		 $(objpfx)tst-fnmatch-mem.out $(objpfx)bug-regex36-mem.out
> +		 $(objpfx)tst-fnmatch-mem.out $(objpfx)bug-regex36-mem.out \
> +		 $(objpfx)tst-glob-tilde-mem.out

OK.

>  xtests-special += $(objpfx)bug-ga2-mem.out
>  endif
>  
> @@ -352,6 +354,12 @@ $(objpfx)bug-glob2-mem.out: $(objpfx)bug-glob2.out
>  	$(common-objpfx)malloc/mtrace $(objpfx)bug-glob2.mtrace > $@; \
>  	$(evaluate-test)
>  
> +tst-glob-tilde-ENV = MALLOC_TRACE=$(objpfx)tst-glob-tilde.mtrace
> +
> +$(objpfx)tst-glob-tilde-mem.out: $(objpfx)tst-glob-tilde.out
> +	$(common-objpfx)malloc/mtrace $(objpfx)tst-glob-tilde.mtrace > $@; \
> +	$(evaluate-test)

OK.

> +
>  $(inst_libexecdir)/getconf: $(inst_bindir)/getconf \
>  			    $(objpfx)getconf.speclist FORCE
>  	$(addprefix $(..)./scripts/mkinstalldirs ,\
> diff --git a/posix/tst-glob-tilde.c b/posix/tst-glob-tilde.c
> new file mode 100644
> index 0000000000..b12d09cf8a
> --- /dev/null
> +++ b/posix/tst-glob-tilde.c
> @@ -0,0 +1,132 @@
> +/* Check for GLOB_TIDLE heap allocation issues (bug 22320, bug 22325).
> +   Copyright (C) 2017 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <glob.h>
> +#include <mcheck.h>
> +#include <nss.h>
> +#include <pwd.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <support/check.h>
> +#include <support/support.h>
> +
> +/* Flag which indicates whether to pass the GLOB_ONLYDIR flag.  */
> +static int do_onlydir;
> +
> +/* Flag which indicates whether to pass the GLOB_NOCHECK flag.  */
> +static int do_nocheck;
> +
> +/* Flag which indicates whether to pass the GLOB_MARK flag.  */
> +static int do_mark;
> +
> +static void
> +one_test (const char *prefix, const char *middle, const char *suffix)
> +{
> +  char *pattern = xasprintf ("%s%s%s", prefix, middle, suffix);
> +  int flags = GLOB_TILDE;
> +  if (do_onlydir)
> +    flags |= GLOB_ONLYDIR;
> +  if (do_nocheck)
> +    flags |= GLOB_NOCHECK;
> +  if (do_mark)
> +    flags |= GLOB_MARK;
> +  glob_t gl;
> +  /* This glob call might result in crashes or memory leaks.  */
> +  if (glob (pattern, flags, NULL, &gl) == 0)
> +    globfree (&gl);

OK.

> +  free (pattern);
> +}
> +
> +enum
> +  {
> +    /* The largest base being tested.  */
> +    largest_base_size = 500000,
> +
> +    /* The actual size is the base size plus a variable whose absolute
> +       value is not greater than this.  This helps malloc to trigger
> +       overflows.  */
> +    max_size_skew = 16,
> +
> +    /* The maximum string length supported by repeating_string
> +       below.  */
> +    repeat_size = largest_base_size + max_size_skew,
> +  };

OK.

> +
> +/* Used to construct strings which repeat a single character 'x'.  */
> +static char *repeat;
> +
> +/* Return a string of SIZE characters.  */
> +const char *
> +repeating_string (int size)
> +{
> +  TEST_VERIFY (size >= 0);
> +  TEST_VERIFY (size <= repeat_size);
> +  const char *repeated_shifted = repeat + repeat_size - size;
> +  TEST_VERIFY (strlen (repeated_shifted) == size);
> +  return repeated_shifted;

OK.

> +}
> +
> +static int
> +do_test (void)
> +{
> +  /* Avoid network-based NSS modules and initialize nss_files with a
> +     dummy lookup.  This has to come before mtrace because NSS does
> +     not free all memory.  */
> +  __nss_configure_lookup ("passwd", "files");
> +  (void) getpwnam ("root");

OK. Ugh ;-)

> +
> +  mtrace ();
> +
> +  repeat = xmalloc (repeat_size + 1);
> +  memset (repeat, 'x', repeat_size);
> +  repeat[repeat_size] = '\0';
> +
> +  static const int base_sizes[] = { 0, 8, 100, 100000, largest_base_size, -1 };
> +

This needs a comment explaining what we are testing and why.

> +  for (do_onlydir = 0; do_onlydir < 2; ++do_onlydir)
> +    for (do_nocheck = 0; do_nocheck < 2; ++do_nocheck)
> +      for (do_mark = 0; do_mark < 2; ++do_mark)
> +        for (int base_idx = 0; base_sizes[base_idx] >= 0; ++base_idx)
> +          {
> +            for (int size_skew = -max_size_skew; size_skew <= max_size_skew;
> +                 ++size_skew)
> +              {
> +                int size = base_sizes[base_idx] + size_skew;
> +                if (size < 0)
> +                  continue;
> +
> +                const char *user_name = repeating_string (size);
> +                one_test ("~", user_name, "/a/b");
> +              }
> +
> +            const char *user_name = repeating_string (base_sizes[base_idx]);
> +            one_test ("~", user_name, "");
> +            one_test ("~", user_name, "/");
> +            one_test ("~", user_name, "/a");
> +            one_test ("~", user_name, "/*/*");
> +            one_test ("~", user_name, "\\/");
> +            one_test ("/~", user_name, "");
> +            one_test ("*/~", user_name, "/a/b");
> +          }
> +
> +  free (repeat);
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> 


-- 
Cheers,
Carlos.