From 26c6620f1799962b5f31cb8d66e79d73762fd160 Mon Sep 17 00:00:00 2001 From: Jim Meyering Date: Sat, 11 May 2013 18:43:50 -0700 Subject: [PATCH] quotearg: do not read beyond end of buffer * lib/quotearg.c (quotearg_buffer_restyled): Do not read beyond the end of an ARG for which no length was specified. With an N-byte quote string, (e.g., N is 3 in the fr_FR.UTF-8 locale), this function would read N-2 bytes beyond ARG's trailing NUL. This was triggered via coreutils' misc/sort-debug-keys.sh test and detected by running the test against a binary compiled with gcc-4.8.0's -fsanitize=address. * tests/test-quotearg-simple.c (main): Add a test to trigger the bug. * modules/quotearg-simple-tests (Files): Add tests/zerosize-ptr.h. Introduced via the 2000-01-15 commit, c4b7f3f8, "Quote multibyte characters correctly." --- ChangeLog | 14 ++++++++++++++ lib/quotearg.c | 7 ++++++- modules/quotearg-simple-tests | 6 ++++++ tests/test-quotearg-simple.c | 35 +++++++++++++++++++++++++++++++++++ 4 files changed, 61 insertions(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index 94cc0d8b9..08f2fe0bf 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,17 @@ +2013-05-11 Jim Meyering + + quotearg: do not read beyond end of buffer + * lib/quotearg.c (quotearg_buffer_restyled): Do not read beyond the + end of an ARG for which no length was specified. With an N-byte + quote string, (e.g., N is 3 in the fr_FR.UTF-8 locale), this function + would read N-2 bytes beyond ARG's trailing NUL. This was triggered + via coreutils' misc/sort-debug-keys.sh test and detected by running + the test against a binary compiled with gcc-4.8.0's -fsanitize=address. + * tests/test-quotearg-simple.c (main): Add a test to trigger the bug. + * modules/quotearg-simple-tests (Files): Add tests/zerosize-ptr.h. + Introduced via the 2000-01-15 commit, c4b7f3f8, "Quote multibyte + characters correctly." + 2013-05-11 Daiki Ueno lock: work around pthread recursive mutexes bug in Mac OS X 10.6 diff --git a/lib/quotearg.c b/lib/quotearg.c index 57a838223..40114d7fb 100644 --- a/lib/quotearg.c +++ b/lib/quotearg.c @@ -348,7 +348,12 @@ quotearg_buffer_restyled (char *buffer, size_t buffersize, if (backslash_escapes && quote_string_len - && i + quote_string_len <= argsize + && (i + quote_string_len + <= (argsize == SIZE_MAX && 1 < quote_string_len + /* Use strlen only if we must: when argsize is SIZE_MAX, + and when the quote string is more than 1 byte long. + If we do call strlen, save the result. */ + ? (argsize = strlen (arg)) : argsize)) && memcmp (arg + i, quote_string, quote_string_len) == 0) { if (elide_outer_quotes) diff --git a/modules/quotearg-simple-tests b/modules/quotearg-simple-tests index 0ef151d1a..e27bf247e 100644 --- a/modules/quotearg-simple-tests +++ b/modules/quotearg-simple-tests @@ -2,12 +2,18 @@ Files: tests/test-quotearg-simple.c tests/test-quotearg.h tests/macros.h +tests/zerosize-ptr.h Depends-on: progname stdint configure.ac: +dnl Check for prerequisites for memory fence checks. +dnl FIXME: zerosize-ptr.h requires these: make a module for it +gl_FUNC_MMAP_ANON +AC_CHECK_HEADERS_ONCE([sys/mman.h]) +AC_CHECK_FUNCS_ONCE([mprotect]) Makefile.am: TESTS += test-quotearg-simple diff --git a/tests/test-quotearg-simple.c b/tests/test-quotearg-simple.c index e7aa8fb68..fe860ed8b 100644 --- a/tests/test-quotearg-simple.c +++ b/tests/test-quotearg-simple.c @@ -29,6 +29,7 @@ #include "localcharset.h" #include "progname.h" #include "macros.h" +#include "zerosize-ptr.h" #include "test-quotearg.h" @@ -297,6 +298,40 @@ main (int argc _GL_UNUSED, char *argv[]) ascii_only); } + { + /* Trigger the bug whereby quotearg_buffer would read beyond the NUL + that defines the end of the string being quoted. Use an input + string whose NUL is the last byte before an unreadable page. */ + char *z = zerosize_ptr (); + + if (z) + { + size_t q_len = 1024; + char *q = malloc (q_len + 1); + char buf[10]; + memset (q, 'Q', q_len); + q[q_len] = 0; + + /* Z points to the boundary between a readable/writable page + and one that is neither readable nor writable. Position + our string so its NUL is at the end of the writable one. */ + char const *str = "____"; + size_t s_len = strlen (str); + z -= s_len + 1; + memcpy (z, str, s_len + 1); + + set_custom_quoting (NULL, q, q); + /* Whether this actually triggers a SEGV depends on the + implementation of memcmp: whether it compares only byte-at- + a-time, and from left to right (no SEGV) or some other way. */ + size_t n = quotearg_buffer (buf, sizeof buf, z, SIZE_MAX, NULL); + ASSERT (n == s_len + 2 * q_len); + ASSERT (memcmp (buf, q, sizeof buf) == 0); + free (q); + } + } + quotearg_free (); + return 0; } -- 2.11.0