[tor-commits] [tor/master] Another 10363 instance: this one in tor_memmem fallback code

[nickm at torproject.org]

commit 9dd115d6b59aaa7d2f444efb9d7992e472fbfb0f
Author: Nick Mathewson <nickm at torproject.org>
Date:   Wed Dec 11 15:14:43 2013 -0500

    Another 10363 instance: this one in tor_memmem fallback code
---
 changes/bug10363     |    3 +++
 src/common/compat.c  |   20 ++++++++++++++------
 src/test/test_util.c |    4 ++++
 3 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/changes/bug10363 b/changes/bug10363
index cf5b7fd..a3c9905 100644
--- a/changes/bug10363
+++ b/changes/bug10363
@@ -3,4 +3,7 @@
       that could, under unlucky circumstances, have led to a pointer
       overflow. Fixes bug #10363; bugfixes on 0.2.0.10-alpha and
       0.2.3.6-alpha. Reported by "bobnomnom".
+    - Fix another possibly undefined pointer operations in tor_memmem
+      fallback implementation. Another case of bug #10363; bugfix on
+      0.1.1.1-alpha.
 
diff --git a/src/common/compat.c b/src/common/compat.c
index d88c5f9..9b8a0a4 100644
--- a/src/common/compat.c
+++ b/src/common/compat.c
@@ -498,21 +498,29 @@ tor_memmem(const void *_haystack, size_t hlen,
 #else
   /* This isn't as fast as the GLIBC implementation, but it doesn't need to
    * be. */
-  const char *p, *end;
+  const char *p, *last_possible_start;
   const char *haystack = (const char*)_haystack;
   const char *needle = (const char*)_needle;
   char first;
   tor_assert(nlen);
 
+  if (nlen > hlen)
+    return NULL;
+
   p = haystack;
-  end = haystack + hlen;
+  /* Last position at which the needle could start. */
+  last_possible_start = haystack + hlen - nlen;
   first = *(const char*)needle;
-  while ((p = memchr(p, first, end-p))) {
-    if (p+nlen > end)
-      return NULL;
+  while ((p = memchr(p, first, last_possible_start + 1 - p))) {
     if (fast_memeq(p, needle, nlen))
       return p;
-    ++p;
+    if (++p > last_possible_start) {
+      /* This comparison shouldn't be necessary, since if p was previously
+       * equal to last_possible_start, the next memchr call would be
+       * "memchr(p, first, 0)", which will return NULL. But it clarifies the
+       * logic. */
+      return NULL;
+    }
   }
   return NULL;
 #endif
diff --git a/src/test/test_util.c b/src/test/test_util.c
index 65d9d2f..bf87db7 100644
--- a/src/test/test_util.c
+++ b/src/test/test_util.c
@@ -1045,6 +1045,10 @@ test_util_strmisc(void)
     test_assert(!tor_memmem(haystack, 4, "cde", 3));
     haystack = "ababcad";
     test_eq_ptr(tor_memmem(haystack, 7, "abc", 3), haystack + 2);
+    test_eq_ptr(tor_memmem(haystack, 7, "ad", 2), haystack + 5);
+    test_eq_ptr(tor_memmem(haystack, 7, "cad", 3), haystack + 4);
+    test_assert(!tor_memmem(haystack, 7, "dadad", 5));
+    test_assert(!tor_memmem(haystack, 7, "abcdefghij", 10));
     /* memstr */
     test_eq_ptr(tor_memstr(haystack, 7, "abc"), haystack + 2);
     test_eq_ptr(tor_memstr(haystack, 7, "cad"), haystack + 4);