[sanitizers] Fix interception of GLibc regexec

[arichardson]

Summary

Previously, on GLibc systems, the interceptor was calling compat_regexec
(regexec@GLIBC_2.2.5) insead of the newer regexec (regexec@GLIBC_2.3.4).
The __compat_regexec strips the REG_STARTEND flag but does not report an
error if other flags are present. This can result in infinite loops for
programs that use REG_STARTEND to find all matches inside a buffer (since
ignoring REG_STARTEND means that the search always starts from the first
character).

The underlying issue is that GLibc's dlsym(RTLD_NEXT, ...) appears to
always return the oldest versioned symbol instead of the default. This
means it does not match the behaviour of dlsym(RTLD_DEFAULT, ...) or the
behaviour documented in the manpage.

It appears a similar issue was encountered with realpath and worked around
in rG77ef78a0a5dbaa364529bd05ed7a7bd9a71dd8d4.

See also https://sourceware.org/bugzilla/show_bug.cgi?id=14932 and
https://sourceware.org/bugzilla/show_bug.cgi?id=1319.

Fixes https://github.com/google/sanitizers/issues/1371

[vitalybuka]

compiler-rt/test/sanitizer_common/TestCases/Posix/regex_startend.cpp

2

Test can be simplified by removing FileCheck and just using asserts

[arichardson]

• simplify test using abort()

Note: incremental ninja check-sanitizer builds will fail this test since the change to the .inc does not trigger a rebuild of the runtime libraries. CMake bug?

[vitalybuka]

In D96348#2568009, @arichardson wrote:

• simplify test using abort()

Note: incremental ninja check-sanitizer builds will fail this test since the change to the .inc does not trigger a rebuild of the runtime libraries. CMake bug?

it's possible

compiler-rt/test/sanitizer_common/TestCases/Posix/regex_startend.cpp

26–39

I assumed you can remove printfs and most of this ifs and use asserts

[arichardson]

compiler-rt/test/sanitizer_common/TestCases/Posix/regex_startend.cpp

26–39

Yeah I can definitely do this, but I find the prints quite useful when things do go wrong. Happy to change it if you prefer,

[vitalybuka]

compiler-rt/test/sanitizer_common/TestCases/Posix/regex_startend.cpp

26–39

assert(expected != nullptr);
assert((size_t)matchlen == strlen(expected));
assert(strncmp(matchstart, expected, matchlen) == 0);

[marxin]

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

7790

I think this is problematic because there are targets that don't define regexec@GLIBC_2.3.4.
Please see my target selection in https://reviews.llvm.org/D95864.

[arichardson]

• Update to do an unversioned fallback (needed for non-x86_64, see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98920)

[arichardson]

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

7790

I've added logic to do an unversioned fallback. To me this seems a bit more maintainable than encoding the target list (since that needs to be updated for every new architecture).

[ro]

The new test FAILs on Solaris:

SanitizerCommon-asan-i386-SunOS :: Posix/regex_startend.cpp
SanitizerCommon-ubsan-i386-SunOS :: Posix/regex_startend.cpp
SanitizerCommon-ubsan-x86_64-SunOS :: Posix/regex_startend.cpp

with

/vol/llvm/src/llvm-project/dist/compiler-rt/test/sanitizer_common/TestCases/Posix/regex_startend.cpp:22:44: error: use of undeclared identifier 'REG_STARTEND'
  int rv = regexec(preg, string, 1, match, REG_STARTEND);

This is no wonder given Linux regex(3) states

REG_STARTEND
       Use   pmatch[0]   on   the   input   string,  starting  at  byte
       pmatch[0].rm_so and ending before  byte  pmatch[0].rm_eo.   This
       allows  matching  embedded  NUL  bytes and avoids a strlen(3) on
       large strings.  It does not use nmatch on input,  and  does  not
       change REG_NOTBOL or REG_NEWLINE processing.  This flag is a BSD
       extension, not present in POSIX.

If the test requires glibc targets, it should be restricted to such.

[arichardson]

In D96348#2613433, @ro wrote:

The new test FAILs on Solaris:

SanitizerCommon-asan-i386-SunOS :: Posix/regex_startend.cpp
SanitizerCommon-ubsan-i386-SunOS :: Posix/regex_startend.cpp
SanitizerCommon-ubsan-x86_64-SunOS :: Posix/regex_startend.cpp

with

/vol/llvm/src/llvm-project/dist/compiler-rt/test/sanitizer_common/TestCases/Posix/regex_startend.cpp:22:44: error: use of undeclared identifier 'REG_STARTEND'
  int rv = regexec(preg, string, 1, match, REG_STARTEND);

This is no wonder given Linux regex(3) states

REG_STARTEND
       Use   pmatch[0]   on   the   input   string,  starting  at  byte
       pmatch[0].rm_so and ending before  byte  pmatch[0].rm_eo.   This
       allows  matching  embedded  NUL  bytes and avoids a strlen(3) on
       large strings.  It does not use nmatch on input,  and  does  not
       change REG_NOTBOL or REG_NEWLINE processing.  This flag is a BSD
       extension, not present in POSIX.

If the test requires glibc targets, it should be restricted to such.

It is a common extension that is present on all BSDs, macOS, Linux and possibly other operating systems. It think the best solution would be to add UNSUPPORTED: solaris or wrap the entire test in #ifdef REG_STARTEND and add a dummy main method?

[ro]

In D96348#2613440, @arichardson wrote:

It is a common extension that is present on all BSDs, macOS, Linux and possibly other operating systems. It think the best solution would be to add UNSUPPORTED: solaris or wrap the entire test in #ifdef REG_STARTEND and add a dummy main method?

It may be common, but it's nonstandard and certainly not univeral: I've just checked AIX 7.2 which doesn't have it either. I believe wrapping in #ifdef REG_STARTEND is better than repeatedly adjusting UNSUPPORTED clauses, provided the test can work on every target that has REG_STARTEND.

[arichardson]

In D96348#2613450, @ro wrote:

In D96348#2613440, @arichardson wrote:

It is a common extension that is present on all BSDs, macOS, Linux and possibly other operating systems. It think the best solution would be to add UNSUPPORTED: solaris or wrap the entire test in #ifdef REG_STARTEND and add a dummy main method?

It may be common, but it's nonstandard and certainly not univeral: I've just checked AIX 7.2 which doesn't have it either. I believe wrapping in #ifdef REG_STARTEND is better than repeatedly adjusting UNSUPPORTED clauses, provided the test can work on every target that has REG_STARTEND.

The test will work on all targets where the host libc supports REG_STARTEND (unless of course there is a bug related to sanitizer interception as was the case on x86_64 glibc).

[ro]

In D96348#2613450, @ro wrote:

In D96348#2613440, @arichardson wrote:

It is a common extension that is present on all BSDs, macOS, Linux and possibly other operating systems. It think the best solution would be to add UNSUPPORTED: solaris or wrap the entire test in #ifdef REG_STARTEND and add a dummy main method?

It may be common, but it's nonstandard and certainly not univeral: I've just checked AIX 7.2 which doesn't have it either. I believe wrapping in #ifdef REG_STARTEND is better than repeatedly adjusting UNSUPPORTED clauses, provided the test can work on every target that has REG_STARTEND.

Done now as D98425.

[marxin]

I would like to point to an issue reported on s390x:
https://github.com/google/sanitizers/issues/1390

[MaskRay]

The underlying issue is that GLibc's dlsym(RTLD_NEXT, ...) appears to always return the oldest versioned symbol instead of the default. This means it does not match the behaviour of dlsym(RTLD_DEFAULT, ...) or the behaviour documented in the manpage.

I fixed https://sourceware.org/bugzilla/show_bug.cgi?id=14932 today, so we shall not need versioned interceptors in the future (glibc >= 2.36).
The current versioned interceptors are still needed for glibc<2.36 compatibility.

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

 #ifdef SANITIZER_NLDBL_VERSION
 #define COMMON_INTERCEPT_FUNCTION_LDBL(fn)                          \
     COMMON_INTERCEPT_FUNCTION_VER(fn, SANITIZER_NLDBL_VERSION)
 #else
 #define COMMON_INTERCEPT_FUNCTION_LDBL(fn)                          \
     COMMON_INTERCEPT_FUNCTION(fn)
 #endif
 
+#if SANITIZER_GLIBC
+#define COMMON_INTERCEPT_FUNCTION_GLIBC_VER(fn, ver) \
+  COMMON_INTERCEPT_FUNCTION_VER(fn, ver)
+#else
+#define COMMON_INTERCEPT_FUNCTION_GLIBC_VER(fn, ver) \
+  COMMON_INTERCEPT_FUNCTION(fn)
+#endif
+
 #ifndef COMMON_INTERCEPTOR_MEMSET_IMPL
 #define COMMON_INTERCEPTOR_MEMSET_IMPL(ctx, dst, v, size) \
   {                                                       \
     if (COMMON_INTERCEPTOR_NOTHING_IS_INITIALIZED)        \
       return internal_memset(dst, v, size);               \
     COMMON_INTERCEPTOR_ENTER(ctx, memset, dst, v, size);  \
     if (common_flags()->intercept_intrin)                 \
       COMMON_INTERCEPTOR_WRITE_RANGE(ctx, dst, size);     \
enum {
     CIMT_FILE
   } type;
   union {
     FileMetadata file;
   };
 };
 
 #if SI_POSIX
 typedef AddrHashMap<CommonInterceptorMetadata, 31051> MetadataHashMap;
 
 static MetadataHashMap *interceptor_metadata_map;
 
 UNUSED static void SetInterceptorMetadata(__sanitizer_FILE *addr,
                                           const FileMetadata &file) {
   MetadataHashMap::Handle h(interceptor_metadata_map, (uptr)addr);
   CHECK(h.created());
   h->type = CommonInterceptorMetadata::CIMT_FILE;
   h->file = file;
 }
 
 UNUSED static const FileMetadata *GetInterceptorMetadata(
     __sanitizer_FILE *addr) {

 
 UNUSED static void DeleteInterceptorMetadata(void *addr) {
   MetadataHashMap::Handle h(interceptor_metadata_map, (uptr)addr, true);
   CHECK(h.exists());
 }
 #endif  // SI_POSIX
 
 #if SANITIZER_INTERCEPT_STRLEN
 INTERCEPTOR(SIZE_T, strlen, const char *s) {
   // Sometimes strlen is called prior to InitializeCommonInterceptors,
   // in which case the REAL(strlen) typically used in
   // COMMON_INTERCEPTOR_ENTER will fail.  We use internal_strlen here
   // to handle that.
   if (COMMON_INTERCEPTOR_NOTHING_IS_INITIALIZED)
     return internal_strlen(s);
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, strlen, s);
   SIZE_T result = REAL(strlen)(s);
   if (common_flags()->intercept_strlen)
     COMMON_INTERCEPTOR_READ_RANGE(ctx, s, result + 1);
   return result;
 }
 #define INIT_STRLEN COMMON_INTERCEPT_FUNCTION(strlen)
 #else
 #define INIT_STRLEN
 #endif
 
 #if SANITIZER_INTERCEPT_STRNLEN
 INTERCEPTOR(SIZE_T, strnlen, const char *s, SIZE_T maxlen) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, strnlen, s, maxlen);
   SIZE_T length = REAL(strnlen)(s, maxlen);
   if (common_flags()->intercept_strlen)
     COMMON_INTERCEPTOR_READ_RANGE(ctx, s, Min(length + 1, maxlen));
   return length;
 }
 #define INIT_STRNLEN COMMON_INTERCEPT_FUNCTION(strnlen)
 #else
 #define INIT_STRNLEN
 #endif
 
 #if SANITIZER_INTERCEPT_STRNDUP
 INTERCEPTOR(char*, strndup, const char *s, uptr size) {
   void *ctx;
   COMMON_INTERCEPTOR_STRNDUP_IMPL(ctx, s, size);
 }
 #define INIT_STRNDUP COMMON_INTERCEPT_FUNCTION(strndup)
 #else
 #define INIT_STRNDUP
 #endif // SANITIZER_INTERCEPT_STRNDUP
 
INTERCEPTOR(void, regfree, const void *preg) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, regfree, preg);
   if (preg)
     COMMON_INTERCEPTOR_READ_RANGE(ctx, preg, struct_regex_sz);
   REAL(regfree)(preg);
 }
 #define INIT_REGEX                                                             \
   COMMON_INTERCEPT_FUNCTION(regcomp);                                          \
-  COMMON_INTERCEPT_FUNCTION(regexec);                                          \
+  COMMON_INTERCEPT_FUNCTION_GLIBC_VER(regexec, "GLIBC_2.3.4");                 \
   COMMON_INTERCEPT_FUNCTION(regerror);                                         \
   COMMON_INTERCEPT_FUNCTION(regfree);
 #else
 #define INIT_REGEX
 #endif
 
 #if SANITIZER_INTERCEPT_REGEXSUB
 INTERCEPTOR(SSIZE_T, regnsub, char *buf, SIZE_T bufsiz, const char *sub,

compiler-rt/test/sanitizer_common/TestCases/Posix/regex_startend.cpp

+// RUN: %clangxx -O0 -g %s -o %t && %run %t
+/// Check that REG_STARTEND is handled correctly.
+/// This is a regression test for https://github.com/google/sanitizers/issues/1371
+/// Previously, on GLibc systems, the interceptor was calling __compat_regexec
+/// (regexec@GLIBC_2.2.5) insead of the newer __regexec (regexec@GLIBC_2.3.4).
+/// The __compat_regexec strips the REG_STARTEND flag but does not report an error
+/// if other flags are present. This can result in infinite loops for programs that
+/// use REG_STARTEND to find all matches inside a buffer (since ignoring REG_STARTEND
+/// means that the search always starts from the first character).
+
+#include <assert.h>
+#include <regex.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+void test_matched(const regex_t *preg, const char *string, size_t start,
+                  size_t end, const char *expected) {
+  regmatch_t match[1];
+  match[0].rm_so = start;
+  match[0].rm_eo = end;
+  int rv = regexec(preg, string, 1, match, REG_STARTEND);
+  int matchlen = (int)(match[0].rm_eo - match[0].rm_so);
+  const char *matchstart = string + match[0].rm_so;
+  if (rv == 0) {
+    printf("%s[%zd,%zd): matched: '%.*s'\n", string, start, end,
+           matchlen, matchstart);
+    if (expected == nullptr) {
+      printf("ERROR: expected no match but got '%.*s'\n",
+             matchlen, matchstart);
+      abort();
+    } else if ((size_t)matchlen != strlen(expected) ||
+               memcmp(matchstart, expected, strlen(expected)) != 0) {
+      printf("ERROR: expected '%s' match but got '%.*s'\n",
+             expected, matchlen, matchstart);
+      abort();
+    } else {
+      printf("SUCCESS\n");
+    }
+  } else if (rv == REG_NOMATCH) {
+    printf("%s[%zd,%zd): no match\n", string, start, end);
+    if (expected != nullptr) {
+      printf("ERROR: expected '%s' match but got no match\n", expected);
+      abort();
+    } else {
+      printf("SUCCESS\n");
+    }
+  } else {
+    printf("ERROR: unexpected regexec return value %d\n", rv);
+    abort();
+  }
+}
+
+int main(void) {
+  printf("Started test\n");
+  regex_t regex;
+  int rv = regcomp(&regex, "[A-Z][A-Z]", 0);
+  assert(!rv);
+  test_matched(&regex, "ABCD", 0, 4, "AB");
+  test_matched(&regex, "ABCD", 0, 1, nullptr); // Not long enough
+  test_matched(&regex, "ABCD", 1, 4, "BC");
+  test_matched(&regex, "ABCD", 1, 2, nullptr); // Not long enough
+  test_matched(&regex, "ABCD", 2, 4, "CD");
+  test_matched(&regex, "ABCD", 2, 3, nullptr); // Not long enough
+  test_matched(&regex, "ABCD", 3, 4, nullptr); // Not long enough
+  regfree(&regex);
+  printf("Finished test\n");
+  return 0;
+}