bug-gnulib
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

broken memchr (was: [PATCH] tests/test-strstr.c: Add another self-test.)


From: Eric Blake
Subject: broken memchr (was: [PATCH] tests/test-strstr.c: Add another self-test.)
Date: Tue, 26 May 2009 16:07:45 +0000 (UTC)
User-agent: Loom/3.14 (http://gmane.org/)

Simon Josefsson <simon <at> josefsson.org> writes:

> >> +  * tests/test-strstr.c: Add another self-test.
> >>    {
> >> +    /* See http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=521737 */
> >> +    char *input = strdup ("aBaaaaaaaaaaax");
> >
> > Did you take the proper precautions to ensure strdup is available?  I'd
> > almost feel better if you rewrote this to use malloc/strcpy, rather than
> > dragging in dependencies just for the test.
> 
> Good point.  Pushed.

Thanks.  But thinking about it more, this doesn't tickle the memchr bug unless 
you are running under efence or other memory checker, so even on platforms 
where the bug is still present, we are not likely to get failure reports.  
Remember, the bug is that memchr dereferences too much memory when the result 
is found early, but this is only fatal if the dereferenced memory was 
unreadable in the first place.  What would be better would be using Bruno's 
recent addition of tests/zerosize-ptr.h as a starting point to write code that 
allocates a 2-page fenced memory region ourselves, then placing the problematic 
string such that the trailing '\0' is the last dereferenceable byte before the 
protected page, so that we can expose the alpha-specific glibc bug in memchr 
even without the use of efence.

And with the recent thread of memchr also being broken on x86_64, I'm starting 
to think that we need to do something like this.  I don't have access to either 
x86_64 or alpha, so I don't know if this is sufficient to detect the broken 
memchr implementations out there; comments are welcome on whether we should 
polish and apply this patch.


From: Eric Blake <address@hidden>
Date: Tue, 26 May 2009 10:03:26 -0600
Subject: [PATCH] memchr: detect broken x86_64 and alpha implementations

* modules/memchr-tests (Depends-on): Move mmap detection...
* modules/memchr (Depends-on): ...here.
(Status, Notice): Delete, this module is no longer obsolete.
(configure.ac): Set indicator.
* lib/string.in.h (memchr): Declare replacement.
* modules/string (Makefile.am): Trigger replacement.
* m4/string_h.m4 (gl_HEADER_STRING_H_DEFAULTS): Likewise.
* m4/memchr.m4 (gl_FUNC_MEMCHR): Use mmap to detect platform
bugs.
* doc/posix-functions/memchr.texi (memchr): Document the bug.
* modules/getpagesize (License): Relax license.
* tests/test-memchr.c (main): Test for the alpha bug.
 in Debian bug 521737 as affecting strstr.

Signed-off-by: Eric Blake <address@hidden>
---
 ChangeLog                       |   21 +++++++++++-
 doc/posix-functions/memchr.texi |    4 ++
 lib/string.in.h                 |   19 ++++++++++-
 m4/memchr.m4                    |   69 ++++++++++++++++++++++++++++++++++++++-
 m4/string_h.m4                  |    6 ++-
 modules/getpagesize             |    2 +-
 modules/memchr                  |   12 +++----
 modules/memchr-tests            |    6 ---
 modules/string                  |    2 +
 tests/test-memchr.c             |   12 +++++++
 10 files changed, 133 insertions(+), 20 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 5b5348c..78df34f 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,20 @@
+2009-05-26  Eric Blake  <address@hidden>
+
+       memchr: detect broken x86_64 and alpha implementations
+       * modules/memchr-tests (Depends-on): Move mmap detection...
+       * modules/memchr (Depends-on): ...here.
+       (Status, Notice): Delete, this module is no longer obsolete.
+       (configure.ac): Set indicator.
+       * lib/string.in.h (memchr): Declare replacement.
+       * modules/string (Makefile.am): Trigger replacement.
+       * m4/string_h.m4 (gl_HEADER_STRING_H_DEFAULTS): Likewise.
+       * m4/memchr.m4 (gl_FUNC_MEMCHR): Use mmap to detect platform
+       bugs.
+       * doc/posix-functions/memchr.texi (memchr): Document the bug.
+       * modules/getpagesize (License): Relax license.
+       * tests/test-memchr.c (main): Test for the alpha bug.
+       Reported in Debian bug 521737 as affecting strstr.
+
 2009-05-26  Simon Josefsson  <address@hidden>

        * tests/test-strstr.c: Add another self-test.
diff --git a/doc/posix-functions/memchr.texi b/doc/posix-functions/memchr.texi
index f8e33a5..5e0a44f 100644
--- a/doc/posix-functions/memchr.texi
+++ b/doc/posix-functions/memchr.texi
@@ -10,6 +10,10 @@ Portability problems fixed by Gnulib:
 @itemize
 @item
 This function is missing on some older platforms.
+
address@hidden
+This function dereferences too much memory on some platforms:
+glibc on x86_64 or Alpha
 @end itemize

 Portability problems not fixed by Gnulib:
diff --git a/lib/string.in.h b/lib/string.in.h
index 7d508cc..c913cce 100644
--- a/lib/string.in.h
+++ b/lib/string.in.h
@@ -1,6 +1,6 @@
 /* A GNU-like <string.h>.

-   Copyright (C) 1995-1996, 2001-2008 Free Software Foundation, Inc.
+   Copyright (C) 1995-1996, 2001-2009 Free Software Foundation, Inc.

    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
@@ -49,6 +49,23 @@ extern "C" {
 #endif


+/* Return the first instance of C within N bytes of S, or NULL.  */
+#if @GNULIB_MEMCHR@
+# if @REPLACE_MEMCHR@
+#  define memchr rpl_memchr
+# endif
+# if @REPLACE_MEMCHR@
+extern void *memchr (void const *__s, int __c, size_t __n)
+  __attribute__ ((__pure__));
+# endif
+#elif defined GNULIB_POSIXCHECK
+# undef memchr
+# define memchr(s,c,n) \
+    (GL_LINK_WARNING ("memchr has platform-specific bugs - " \
+                      "use gnulib module memchr for portability" ), \
+     memchr (s, c, n))
+#endif
+
 /* Return the first occurrence of NEEDLE in HAYSTACK.  */
 #if @GNULIB_MEMMEM@
 # if @REPLACE_MEMMEM@
diff --git a/m4/memchr.m4 b/m4/memchr.m4
index 53c5380..bcfe869 100644
--- a/m4/memchr.m4
+++ b/m4/memchr.m4
@@ -1,4 +1,4 @@
-# memchr.m4 serial 5
+# memchr.m4 serial 6
 dnl Copyright (C) 2002, 2003, 2004, 2009 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
@@ -6,9 +6,76 @@ dnl with or without modifications, as long as this notice is 
preserved.

 AC_DEFUN([gl_FUNC_MEMCHR],
 [
+  dnl Check for prerequisites for memory fence checks.
+  gl_FUNC_MMAP_ANON
+  AC_CHECK_HEADERS_ONCE([sys/mman.h])
+  AC_CHECK_FUNCS_ONCE([mprotect])
+
+  dnl These days, we assume memchr is present.  But just in case...
+  AC_REQUIRE([gl_HEADER_STRING_H_DEFAULTS])
   AC_REPLACE_FUNCS([memchr])
   if test $ac_cv_func_memchr = no; then
     gl_PREREQ_MEMCHR
+    REPLACE_MEMCHR=1
+  fi
+
+  if test $ac_cv_func_memchr = yes; then
+    # Detect platform-specific bugs in some versions of glibc:
+    # memchr should not dereference anything with length 0
+    #   http://bugzilla.redhat.com/499689
+    # memchr should not dereference overestimated length after a match
+    #   http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=521737
+    # Assume that memchr works on platforms that lack mprotect.
+    AC_CACHE_CHECK([whether memchr works], [gl_cv_func_memchr_works],
+      [AC_RUN_IFELSE([AC_LANG_PROGRAM([[
+#include <string.h>
+#if HAVE_SYS_MMAN_H
+# include <fcntl.h>
+# include <unistd.h>
+# include <sys/types.h>
+# include <sys/mman.h>
+# ifndef MAP_FILE
+#  define MAP_FILE 0
+# endif
+#endif
+]], [[
+  char *fence = NULL;
+#if HAVE_SYS_MMAN_H && HAVE_MPROTECT
+# if HAVE_MAP_ANONYMOUS
+  const int flags = MAP_ANONYMOUS | MAP_PRIVATE;
+  const int fd = -1;
+# else /* !HAVE_MAP_ANONYMOUS */
+  const int flags = MAP_FILE | MAP_PRIVATE;
+  int fd = open ("/dev/zero", O_RDONLY, 0666);
+  if (fd >= 0)
+# endif
+    {
+      int pagesize = getpagesize ();
+      char *two_pages =
+       (char *) mmap (NULL, 2 * pagesize, PROT_READ | PROT_WRITE,
+                      flags, fd, 0);
+      if (two_pages != (char *)(-1)
+         && mprotect (two_pages + pagesize, pagesize, PROT_NONE) == 0)
+       fence = two_pages + pagesize;
+    }
+#endif
+  if (fence)
+    {
+      if (memchr (fence, 0, 0))
+        return 1;
+      strcpy (fence - 9, "12345678");
+      if (memchr (fence - 9, 0, 79) != fence - 1)
+        return 2;
+    }
+  return 0;
+]])], [gl_cv_func_memchr_works=yes], [gl_cv_func_memchr_works=no],
+      [dnl Be pessimistic for now.
+       gl_cv_func_memchr_works="guessing no"])])
+    if test "$gl_cv_func_memchr_works" != yes; then
+      gl_PREREQ_MEMCHR
+      REPLACE_MEMCHR=1
+      AC_LIBOBJ([memchr])
+    fi
   fi
 ])

diff --git a/m4/string_h.m4 b/m4/string_h.m4
index 2d5553c..11f09c8 100644
--- a/m4/string_h.m4
+++ b/m4/string_h.m4
@@ -1,11 +1,11 @@
 # Configure a GNU-like replacement for <string.h>.

-# Copyright (C) 2007, 2008 Free Software Foundation, Inc.
+# Copyright (C) 2007, 2008, 2009 Free Software Foundation, Inc.
 # This file is free software; the Free Software Foundation
 # gives unlimited permission to copy and/or distribute it,
 # with or without modifications, as long as this notice is preserved.

-# serial 6
+# serial 7

 # Written by Paul Eggert.

@@ -32,6 +32,7 @@ AC_DEFUN([gl_STRING_MODULE_INDICATOR],

 AC_DEFUN([gl_HEADER_STRING_H_DEFAULTS],
 [
+  GNULIB_MEMCHR=0;      AC_SUBST([GNULIB_MEMCHR])
   GNULIB_MEMMEM=0;      AC_SUBST([GNULIB_MEMMEM])
   GNULIB_MEMPCPY=0;     AC_SUBST([GNULIB_MEMPCPY])
   GNULIB_MEMRCHR=0;     AC_SUBST([GNULIB_MEMRCHR])
@@ -83,6 +84,7 @@ AC_DEFUN([gl_HEADER_STRING_H_DEFAULTS],
   HAVE_DECL_STRERROR=1;                AC_SUBST([HAVE_DECL_STRERROR])
   HAVE_DECL_STRSIGNAL=1;       AC_SUBST([HAVE_DECL_STRSIGNAL])
   HAVE_STRVERSCMP=1;           AC_SUBST([HAVE_STRVERSCMP])
+  REPLACE_MEMCHR=0;            AC_SUBST([REPLACE_MEMCHR])
   REPLACE_MEMMEM=0;            AC_SUBST([REPLACE_MEMMEM])
   REPLACE_STRDUP=0;            AC_SUBST([REPLACE_STRDUP])
   REPLACE_STRSTR=0;            AC_SUBST([REPLACE_STRSTR])
diff --git a/modules/getpagesize b/modules/getpagesize
index 9cc4f37..6ad1620 100644
--- a/modules/getpagesize
+++ b/modules/getpagesize
@@ -18,7 +18,7 @@ Include:
 <unistd.h>

 License:
-LGPL
+LGPLv2+

 Maintainer:
 Jim Meyering
diff --git a/modules/memchr b/modules/memchr
index 1ec8f41..a77bec5 100644
--- a/modules/memchr
+++ b/modules/memchr
@@ -1,20 +1,19 @@
 Description:
 memchr() function: scan memory for a byte.

-Status:
-obsolete
-
-Notice:
-This module is obsolete.
-
 Files:
 lib/memchr.c
 m4/memchr.m4
+m4/mmap-anon.m4

 Depends-on:
+extensions
+getpagesize
+string

 configure.ac:
 gl_FUNC_MEMCHR
+gl_STRING_MODULE_INDICATOR([memchr])

 Makefile.am:

@@ -26,4 +25,3 @@ LGPLv2+

 Maintainer:
 Jim Meyering, glibc
-
diff --git a/modules/memchr-tests b/modules/memchr-tests
index c2c9e2b..32cdd6d 100644
--- a/modules/memchr-tests
+++ b/modules/memchr-tests
@@ -1,16 +1,10 @@
 Files:
 tests/test-memchr.c
 tests/zerosize-ptr.h
-m4/mmap-anon.m4

 Depends-on:
-extensions
-getpagesize

 configure.ac:
-gl_FUNC_MMAP_ANON
-AC_CHECK_HEADERS_ONCE([sys/mman.h])
-AC_CHECK_FUNCS_ONCE([mprotect])

 Makefile.am:
 TESTS += test-memchr
diff --git a/modules/string b/modules/string
index 6283f9e..b2c802c 100644
--- a/modules/string
+++ b/modules/string
@@ -38,6 +38,7 @@ string.h: string.in.h
              -e 's|@''GNULIB_MBSSPN''@|$(GNULIB_MBSSPN)|g' \
              -e 's|@''GNULIB_MBSSEP''@|$(GNULIB_MBSSEP)|g' \
              -e 's|@''GNULIB_MBSTOK_R''@|$(GNULIB_MBSTOK_R)|g' \
+             -e 's|@''GNULIB_MEMCHR''@|$(GNULIB_MEMCHR)|g' \
              -e 's|@''GNULIB_MEMMEM''@|$(GNULIB_MEMMEM)|g' \
              -e 's|@''GNULIB_MEMPCPY''@|$(GNULIB_MEMPCPY)|g' \
              -e 's|@''GNULIB_MEMRCHR''@|$(GNULIB_MEMRCHR)|g' \
@@ -74,6 +75,7 @@ string.h: string.in.h
              -e 's|@''HAVE_DECL_STRERROR''@|$(HAVE_DECL_STRERROR)|g' \
              -e 's|@''HAVE_DECL_STRSIGNAL''@|$(HAVE_DECL_STRSIGNAL)|g' \
              -e 's|@''HAVE_STRVERSCMP''@|$(HAVE_STRVERSCMP)|g' \
+             -e 's|@''REPLACE_MEMCHR''@|$(REPLACE_MEMCHR)|g' \
              -e 's|@''REPLACE_MEMMEM''@|$(REPLACE_MEMMEM)|g' \
              -e 's|@''REPLACE_STRCASESTR''@|$(REPLACE_STRCASESTR)|g' \
              -e 's|@''REPLACE_STRDUP''@|$(REPLACE_STRDUP)|g' \
diff --git a/tests/test-memchr.c b/tests/test-memchr.c
index bf99a82..4dea17d 100644
--- a/tests/test-memchr.c
+++ b/tests/test-memchr.c
@@ -70,6 +70,18 @@ main ()
   ASSERT (MEMCHR (input, 'f', n) == NULL);
   ASSERT (MEMCHR (input, '\0', n) == NULL);

+  /* Check that overestimating the length does not cause dereferences
+     into pages beyond the match.
+     See http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=521737 */
+  {
+    char *fence = zerosize_ptr ();
+    if (fence)
+      {
+        strcpy (fence - 9, "12345678");
+        ASSERT (MEMCHR (fence - 9, '\0', 79) == fence - 1);
+      }
+  }
+
   /* Check that a very long haystack is handled quickly if the byte is
      found near the beginning.  */
   {
-- 
1.6.2.4







reply via email to

[Prev in Thread] Current Thread [Next in Thread]