On Tue, Nov 29, 2016 at 11:08 PM, David Malcolm <dmalc...@redhat.com> wrote:
> libiberty's implementations of strndup and xstrndup call strlen on
> the input string, and hence can read past the end of the input buffer
> if it isn't zero-terminated (such as is the case in PR c/78498, where
> the input string is from the input.c line cache).
>
> This patch converts them to use strnlen instead (as glibc's
> implementation of them does), avoiding reading more than n bytes
> from the input buffer. strnlen is provided by libiberty.
>
> Successfully bootstrapped®rtested on x86_64-pc-linux-gnu;
> adds 6 PASS results to gcc.sum.
>
> The patch also adds some selftests for this case, which showed
> the problem and the fix nicely via "make selftest-valgrind".
> Unfortunately I had to put these selftests within the gcc
> subdirectory, rather than libiberty, since selftest.h is C++ and
> is itself in the gcc subdirectory. If that's unacceptable, I can
> just drop the selftest.c part of the patch (or we somehow support
> selftests from within libiberty itself, though I'm not sure how to
> do that, if libiberty is meant as a cross-platform compat library,
> rather than as a base support layer; the simplest thing to do seemed
> to be to put them in the "gcc" subdir).
Ok.
Thanks,
Richard.
> gcc/ChangeLog:
> PR c/78498
> * selftest.c (selftest::assert_strndup_eq): New function.
> (selftest::test_strndup): New function.
> (selftest::test_libiberty): New function.
> (selftest::selftest_c_tests): Call test_libiberty.
>
> gcc/testsuite/ChangeLog:
> PR c/78498
> * gcc.dg/format/pr78494.c: New test case.
>
> libiberty/ChangeLog:
> PR c/78498
> * strndup.c (strlen): Delete decl.
> (strnlen): Add decl.
> (strndup): Call strnlen rather than strlen.
> * xstrndup.c: Likewise.
> ---
> gcc/selftest.c | 48
> +++++++++++++++++++++++++++++++++++
> gcc/testsuite/gcc.dg/format/pr78494.c | 12 +++++++++
> libiberty/strndup.c | 7 ++---
> libiberty/xstrndup.c | 5 +---
> 4 files changed, 63 insertions(+), 9 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/format/pr78494.c
>
> diff --git a/gcc/selftest.c b/gcc/selftest.c
> index 2a729be..6df73c2 100644
> --- a/gcc/selftest.c
> +++ b/gcc/selftest.c
> @@ -198,6 +198,53 @@ read_file (const location &loc, const char *path)
> return result;
> }
>
> +/* Selftests for libiberty. */
> +
> +/* Verify that both strndup and xstrndup generate EXPECTED
> + when called on SRC and N. */
> +
> +static void
> +assert_strndup_eq (const char *expected, const char *src, size_t n)
> +{
> + char *buf = strndup (src, n);
> + if (buf)
> + ASSERT_STREQ (expected, buf);
> + free (buf);
> +
> + buf = xstrndup (src, n);
> + ASSERT_STREQ (expected, buf);
> + free (buf);
> +}
> +
> +/* Verify that strndup and xstrndup work as expected. */
> +
> +static void
> +test_strndup ()
> +{
> + assert_strndup_eq ("", "test", 0);
> + assert_strndup_eq ("t", "test", 1);
> + assert_strndup_eq ("te", "test", 2);
> + assert_strndup_eq ("tes", "test", 3);
> + assert_strndup_eq ("test", "test", 4);
> + assert_strndup_eq ("test", "test", 5);
> +
> + /* Test on an string without zero termination. */
> + const char src[4] = {'t', 'e', 's', 't'};
> + assert_strndup_eq ("", src, 0);
> + assert_strndup_eq ("t", src, 1);
> + assert_strndup_eq ("te", src, 2);
> + assert_strndup_eq ("tes", src, 3);
> + assert_strndup_eq ("test", src, 4);
> +}
> +
> +/* Run selftests for libiberty. */
> +
> +static void
> +test_libiberty ()
> +{
> + test_strndup ();
> +}
> +
> /* Selftests for the selftest system itself. */
>
> /* Sanity-check the ASSERT_ macros with various passing cases. */
> @@ -245,6 +292,7 @@ test_read_file ()
> void
> selftest_c_tests ()
> {
> + test_libiberty ();
> test_assertions ();
> test_named_temp_file ();
> test_read_file ();
> diff --git a/gcc/testsuite/gcc.dg/format/pr78494.c
> b/gcc/testsuite/gcc.dg/format/pr78494.c
> new file mode 100644
> index 0000000..4b53a68
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/format/pr78494.c
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -Wall -Wextra -fdiagnostics-show-caret" } */
> +
> +void f (void)
> +{
> + __builtin_printf ("%i", ""); /* { dg-warning "expects argument of type" }
> */
> +/* { dg-begin-multiline-output "" }
> + __builtin_printf ("%i", "");
> + ~^ ~~
> + %s
> + { dg-end-multiline-output "" } */
> +}
> diff --git a/libiberty/strndup.c b/libiberty/strndup.c
> index 9e9b4e2..4556b96 100644
> --- a/libiberty/strndup.c
> +++ b/libiberty/strndup.c
> @@ -33,7 +33,7 @@ memory was available. The result is always NUL terminated.
> #include "ansidecl.h"
> #include <stddef.h>
>
> -extern size_t strlen (const char*);
> +extern size_t strnlen (const char *s, size_t maxlen);
> extern PTR malloc (size_t);
> extern PTR memcpy (PTR, const PTR, size_t);
>
> @@ -41,10 +41,7 @@ char *
> strndup (const char *s, size_t n)
> {
> char *result;
> - size_t len = strlen (s);
> -
> - if (n < len)
> - len = n;
> + size_t len = strnlen (s, n);
>
> result = (char *) malloc (len + 1);
> if (!result)
> diff --git a/libiberty/xstrndup.c b/libiberty/xstrndup.c
> index 0a41f60..c3d2d83 100644
> --- a/libiberty/xstrndup.c
> +++ b/libiberty/xstrndup.c
> @@ -48,10 +48,7 @@ char *
> xstrndup (const char *s, size_t n)
> {
> char *result;
> - size_t len = strlen (s);
> -
> - if (n < len)
> - len = n;
> + size_t len = strnlen (s, n);
>
> result = XNEWVEC (char, len + 1);
>
> --
> 1.8.5.3
>
