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."

diff --git a/ChangeLog b/ChangeLog
index 94cc0d8..08f2fe0 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,17 @@
+2013-05-11  Jim Meyering  <meyering@fb.com>
+
+       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  <ueno@gnu.org>
 
        lock: work around pthread recursive mutexes bug in Mac OS X 10.6

diff --git a/lib/quotearg.c b/lib/quotearg.c
index 57a8382..40114d7 100644 (file)
--- 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 0ef151d..e27bf24 100644 (file)
--- 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 e7aa8fb..fe860ed 100644 (file)
--- 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;
 }