Re: [patch] Refactor wordexp-test

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

On 03/06/2015 08:06 PM, Paul Pluzhnikov wrote:
> Greetings,
> 
> This patch modifies wordexp-test.c such that words always ends at the
> edge of unreadable page.
> 
> This makes it easy to catch overflows, such as BZ #18043 (and BZ #18042).
> 
> Tested: reverted fix for BZ #18043 in wordexp.c and verified that the
> test for it fails with expected SIGSEGV.
> 
> 
> Thanks,
> 
> 2015-03-06  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 
>         * posix/wordexp-test.c (test_case): Add test for BZ #18043
>         (do_bz18043): Delete.
>         (at_page_end): New.
>         (testit): Refactor to have words at the edge of unreadable page.

Amazing.

OK to commit if you fix the nit e.g. use PTR_ALIGN_DOWN or ALIGN_DOWN
to avoid the bespoke alignment code.

> -- Paul Pluzhnikov
> 
> 
> glibc-20150306-bz18043-refactor.patch.txt
> 
> 
> diff --git a/posix/wordexp-test.c b/posix/wordexp-test.c
> index 137044e..b71352d 100644
> --- a/posix/wordexp-test.c
> +++ b/posix/wordexp-test.c
> @@ -233,6 +233,8 @@ struct test_case_struct
>      { WRDE_CMDSUB, NULL, "$((1+`echo 1`))", WRDE_NOCMD, 0, { NULL, }, IFS },
>      { WRDE_CMDSUB, NULL, "$((1+$((`echo 1`))))", WRDE_NOCMD, 0, { NULL, }, IFS },
>  
> +    { WRDE_SYNTAX, NULL, "${", 0, 0, { NULL, }, IFS },  /* BZ 18043  */
> +

OK.

>      { -1, NULL, NULL, 0, 0, { NULL, }, IFS },
>    };
>  
> @@ -250,33 +252,6 @@ command_line_test (const char *words)
>      printf ("we_wordv[%d] = \"%s\"\n", i, we.we_wordv[i]);
>  }
>  
> -static int
> -do_bz18043 (void)
> -{
> -  const int pagesize = getpagesize ();
> -  char *start = mmap (0, 2 * pagesize, PROT_READ|PROT_WRITE,
> -		      MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
> -
> -  if (start == MAP_FAILED)
> -    return 1;
> -
> -  if (mprotect (start + pagesize, pagesize, PROT_NONE))
> -    return 2;
> -
> -  const char word[] = "${";
> -  char *word_start = start + pagesize - sizeof (word);
> -  memcpy (word_start, word, sizeof (word));
> -
> -  wordexp_t w;
> -  if (wordexp (word_start, &w, 0) != WRDE_SYNTAX)
> -    return 3;
> -
> -  if (munmap (start, 2 * pagesize) != 0)
> -    return 4;
> -
> -  return 0;
> -}
> -
>  int
>  main (int argc, char *argv[])
>  {
> @@ -398,12 +373,32 @@ main (int argc, char *argv[])
>  
>    printf ("tests failed: %d\n", fail);
>  
> -  if (do_bz18043 ())
> -    ++fail;
> -
>    return fail != 0;
>  }
>  
> +static const char *
> +at_page_end (const char *words)
> +{
> +  const int pagesize = getpagesize ();
> +  char *start = mmap (0, 2 * pagesize, PROT_READ|PROT_WRITE,
> +		      MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
> +
> +  if (start == MAP_FAILED)
> +    return start;
> +
> +  if (mprotect (start + pagesize, pagesize, PROT_NONE))
> +    {
> +      munmap (start, 2 * pagesize);
> +      return MAP_FAILED;
> +    }
> +
> +  /* Includes terminating NUL.  */
> +  const size_t words_size = strlen (words) + 1;
> +  char *words_start = start + pagesize - words_size;
> +  memcpy (words_start, words, words_size);
> +
> +  return words_start;
> +}

OK.

>  
>  static int
>  testit (struct test_case_struct *tc)
> @@ -431,6 +426,8 @@ testit (struct test_case_struct *tc)
>    we = sav_we;
>  
>    printf ("Test %d (%s): ", ++tests, tc->words);
> +  fflush (NULL);

OK. Good idea to flush before we potentially crash.

> +  const char *words = at_page_end (tc->words);
>  
>    if (tc->flags & WRDE_NOCMD)
>      registered_forks = 0;
> @@ -444,7 +441,7 @@ testit (struct test_case_struct *tc)
>  	  return 1;
>  	}
>      }
> -  retval = wordexp (tc->words, &we, tc->flags);
> +  retval = wordexp (words, &we, tc->flags);

OK. Using a copy.

>  
>    if ((tc->flags & WRDE_NOCMD)
>        && (registered_forks > 0))
> @@ -508,5 +505,11 @@ testit (struct test_case_struct *tc)
>    if (retval == 0 || retval == WRDE_NOSPACE)
>      wordfree (&we);
>  
> +  const int page_size = getpagesize ();
> +  char *start = (char *) (((uintptr_t) words) & ~(page_size - 1));

#include <libc-internal>, use PTR_ALIGN_DOWN().

> +  if (munmap (start, 2 * page_size) != 0)
> +    return 1;
> +
> +  fflush (NULL);
>    return bzzzt;
>  }

Cheers,
Carlos.