[compiler-rt] [win/asan] Ensure errno gets set correctly for strtol (PR #109258)

via llvm-commits llvm-commits at lists.llvm.org
Thu Sep 19 23:58:19 PDT 2024


https://github.com/zmodem updated https://github.com/llvm/llvm-project/pull/109258

>From 21f8761abd9909e9f9afe0819da784060cd20829 Mon Sep 17 00:00:00 2001
From: Hans Wennborg <hans at chromium.org>
Date: Wed, 18 Sep 2024 15:54:54 +0200
Subject: [PATCH] [win/asan] Ensure errno gets set correctly for strtol

This fixes two problems with asan's interception of `strtol` on Windows:

1. In the dynamic runtime, the `strtol` interceptor calls out to ntdll's
   `strtol` to perform the string conversion. Unfortunately, that
   function doesn't set `errno`. This has been a long-standing problem
   (#34485), but it was not an issue when using the static runtime.
   After the static runtime was removed recently (#107899), the problem
   became more urgent.

2. A module linked against the static CRT will have a different instance
   of `errno` than the ASan runtime, since that's now always linked
   against the dynamic CRT. That means even if the ASan runtime sets
   `errno` correctly, the calling module will not see it.

This patch fixes the first problem by making the `strtol` interceptor
call out to `strtoll` instead, and do 32-bit range checks on the result.

I can't think of any reasonable way to fix the second problem, so we
should stop intercepting `strtol` in the static runtime thunk. I checked
the list of functions in the thunk, and `strtol` and `strtoll` are the
only ones that set `errno`. (`strtoll` was already missing, probably by
mistake.)
---
 compiler-rt/lib/asan/asan_interceptors.cpp    | 27 ++++++++++++++++++-
 .../asan/asan_win_static_runtime_thunk.cpp    |  5 +++-
 compiler-rt/lib/asan/tests/asan_str_test.cpp  | 24 +++++++++++++++++
 .../lib/sanitizer_common/sanitizer_errno.cpp  |  1 +
 .../sanitizer_common/sanitizer_errno_codes.h  |  1 +
 .../test/asan/TestCases/strtol_strict.c       |  3 +++
 6 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/compiler-rt/lib/asan/asan_interceptors.cpp b/compiler-rt/lib/asan/asan_interceptors.cpp
index 74af2e65e9bfa6..c13bcf2382b0a3 100644
--- a/compiler-rt/lib/asan/asan_interceptors.cpp
+++ b/compiler-rt/lib/asan/asan_interceptors.cpp
@@ -650,9 +650,34 @@ static ALWAYS_INLINE auto StrtolImpl(void *ctx, Fn real, const char *nptr,
       return StrtolImpl(ctx, REAL(func), nptr, endptr, base);                \
     }
 
-INTERCEPTOR_STRTO_BASE(long, strtol)
 INTERCEPTOR_STRTO_BASE(long long, strtoll)
 
+#  if SANITIZER_WINDOWS
+INTERCEPTOR(long, strtol, const char *nptr, char **endptr, int base) {
+  // REAL(strtol) may be ntdll!strtol, which doesn't set errno. Instead,
+  // call REAL(strtoll) and do the range check ourselves.
+  COMPILER_CHECK(sizeof(long) == sizeof(u32));
+
+  void *ctx;
+  ASAN_INTERCEPTOR_ENTER(ctx, strtol);
+  AsanInitFromRtl();
+
+  long long result = StrtolImpl(ctx, REAL(strtoll), nptr, endptr, base);
+
+  if (result > INT32_MAX) {
+    errno = errno_ERANGE;
+    return INT32_MAX;
+  }
+  if (result < INT32_MIN) {
+    errno = errno_ERANGE;
+    return INT32_MIN;
+  }
+  return (long)result;
+}
+#  else
+INTERCEPTOR_STRTO_BASE(long, strtol)
+#  endif
+
 #  if SANITIZER_GLIBC
 INTERCEPTOR_STRTO_BASE(long, __isoc23_strtol)
 INTERCEPTOR_STRTO_BASE(long long, __isoc23_strtoll)
diff --git a/compiler-rt/lib/asan/asan_win_static_runtime_thunk.cpp b/compiler-rt/lib/asan/asan_win_static_runtime_thunk.cpp
index 9efc693bbf00a2..4a69b66574039c 100644
--- a/compiler-rt/lib/asan/asan_win_static_runtime_thunk.cpp
+++ b/compiler-rt/lib/asan/asan_win_static_runtime_thunk.cpp
@@ -63,10 +63,13 @@ INTERCEPT_LIBRARY_FUNCTION_ASAN(strpbrk);
 INTERCEPT_LIBRARY_FUNCTION_ASAN(strspn);
 INTERCEPT_LIBRARY_FUNCTION_ASAN(strstr);
 INTERCEPT_LIBRARY_FUNCTION_ASAN(strtok);
-INTERCEPT_LIBRARY_FUNCTION_ASAN(strtol);
 INTERCEPT_LIBRARY_FUNCTION_ASAN(wcslen);
 INTERCEPT_LIBRARY_FUNCTION_ASAN(wcsnlen);
 
+// Note: Don't intercept strtol(l). They are supposed to set errno for out-of-
+// range values, but since the ASan runtime is linked against the dynamic CRT,
+// its errno is different from the one in the current module.
+
 #  if defined(_MSC_VER) && !defined(__clang__)
 #    pragma warning(pop)
 #  endif
diff --git a/compiler-rt/lib/asan/tests/asan_str_test.cpp b/compiler-rt/lib/asan/tests/asan_str_test.cpp
index 8b1c1dc459d74f..b28143db860f5e 100644
--- a/compiler-rt/lib/asan/tests/asan_str_test.cpp
+++ b/compiler-rt/lib/asan/tests/asan_str_test.cpp
@@ -638,3 +638,27 @@ TEST(AddressSanitizer, StrtolOOBTest) {
   RunStrtolOOBTest(&CallStrtol);
 }
 #endif
+
+TEST(AddressSanitizer, StrtolOverflow) {
+  if (sizeof(long) == 4) {
+    // Check that errno gets set correctly on 32-bit strtol overflow.
+    long res;
+    errno = 0;
+    res = Ident(strtol("2147483647", NULL, 0));
+    EXPECT_EQ(errno, 0);
+    EXPECT_EQ(res, 2147483647);
+
+    res = Ident(strtol("2147483648", NULL, 0));
+    EXPECT_EQ(errno, ERANGE);
+    EXPECT_EQ(res, 2147483647);
+
+    errno = 0;
+    res = Ident(strtol("-2147483648", NULL, 0));
+    EXPECT_EQ(errno, 0);
+    EXPECT_EQ(res, -2147483648);
+
+    res = Ident(strtol("-2147483649", NULL, 0));
+    EXPECT_EQ(errno, ERANGE);
+    EXPECT_EQ(res, -2147483648);
+  }
+}
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_errno.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_errno.cpp
index cbadf4d924a7e0..a7cdf3256757c7 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_errno.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_errno.cpp
@@ -23,6 +23,7 @@ namespace __sanitizer {
 COMPILER_CHECK(errno_ENOMEM == ENOMEM);
 COMPILER_CHECK(errno_EBUSY == EBUSY);
 COMPILER_CHECK(errno_EINVAL == EINVAL);
+COMPILER_CHECK(errno_ERANGE == ERANGE);
 
 // EOWNERDEAD is not present in some older platforms.
 #if defined(EOWNERDEAD)
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_errno_codes.h b/compiler-rt/lib/sanitizer_common/sanitizer_errno_codes.h
index 3917b2817f2e99..9e6e71ec80c15c 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_errno_codes.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_errno_codes.h
@@ -24,6 +24,7 @@ namespace __sanitizer {
 #define errno_ENOMEM 12
 #define errno_EBUSY 16
 #define errno_EINVAL 22
+#define errno_ERANGE 34
 #define errno_ENAMETOOLONG 36
 #define errno_ENOSYS 38
 
diff --git a/compiler-rt/test/asan/TestCases/strtol_strict.c b/compiler-rt/test/asan/TestCases/strtol_strict.c
index bc30c87b18cb30..aa37af07b63dfc 100644
--- a/compiler-rt/test/asan/TestCases/strtol_strict.c
+++ b/compiler-rt/test/asan/TestCases/strtol_strict.c
@@ -23,6 +23,9 @@
 // RUN: %env_asan_opts=strict_string_checks=true not %run %t test7 2>&1 | FileCheck %s --check-prefix=CHECK7
 // REQUIRES: shadow-scale-3
 
+// On Windows, strtol cannot be intercepted when statically linked against the CRT.
+// UNSUPPORTED: win32-static-asan
+
 #include <assert.h>
 #include <stdlib.h>
 #include <string.h>



More information about the llvm-commits mailing list