Skip to content

Commit

Permalink
[win/asan] Ensure errno gets set correctly for strtol (#109258)
Browse files Browse the repository at this point in the history
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.)
  • Loading branch information
zmodem authored Sep 22, 2024
1 parent d84411f commit 9f3d083
Show file tree
Hide file tree
Showing 6 changed files with 59 additions and 2 deletions.
27 changes: 26 additions & 1 deletion compiler-rt/lib/asan/asan_interceptors.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
5 changes: 4 additions & 1 deletion compiler-rt/lib/asan/asan_win_static_runtime_thunk.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
24 changes: 24 additions & 0 deletions compiler-rt/lib/asan/tests/asan_str_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
1 change: 1 addition & 0 deletions compiler-rt/lib/sanitizer_common/sanitizer_errno.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
1 change: 1 addition & 0 deletions compiler-rt/lib/sanitizer_common/sanitizer_errno_codes.h
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
3 changes: 3 additions & 0 deletions compiler-rt/test/asan/TestCases/strtol_strict.c
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down

0 comments on commit 9f3d083

Please sign in to comment.