[compiler-rt] r186109 - Fix a veritable conucopia of bugs in the readdir_r interceptors.
Chandler Carruth
chandlerc at gmail.com
Thu Jul 11 11:51:40 PDT 2013
Author: chandlerc
Date: Thu Jul 11 13:51:40 2013
New Revision: 186109
URL: http://llvm.org/viewvc/llvm-project?rev=186109&view=rev
Log:
Fix a veritable conucopia of bugs in the readdir_r interceptors.
First, the reason I came here: I forgot to look at readdir64_r which had
the exact same bug as readdir_r. However, upon applying the same
quick-fix and testing it I discovered that it still didn't work at all.
As a consequence, I spent some time studying the code and thinking about
it and fixed several other problems.
Second, the code was checking for a null entry and result pointer, but
there is no indication that null pointers are viable here. Certainly,
the spec makes it extremely clear that there is no non-error case where
the implementation of readdir_r fails to dereference the 'result'
pointer and store NULL to it. Thus, our checking for a non-null 'result'
pointer before reflecting that write in the instrumentation was
trivially dead. Remove it.
Third, the interceptor was marking the write to the actual dirent struct
by looking at the entry pointer, but nothing in the spec requires that
the dirent struct written is actually written into the entry structure
provided. A threadlocal buffer would be just as conforming, and the spec
goes out of its way to say the pointer to the *actual* result dirent
struct is stored into *result, so *that* is where the interceptor should
reflect a write occuring. This also obviates the need to even consider
whether the 'entry' parameter is null.
Fourth, I got to the bottom of why nothing at all worked in readdir64_r
-- the interceptor structure for dirent64 was completely wrong in that
it was the same as dirent. I fixed this struct to be correct (64-bit
inode and 64-bit offset! just a 64-bit offset isn't enough!) and added
several missing tests for the size and layout of this struct.
Modified:
compiler-rt/trunk/lib/asan/lit_tests/TestCases/Linux/interception_readdir_r_test.cc
compiler-rt/trunk/lib/sanitizer_common/sanitizer_common_interceptors.inc
compiler-rt/trunk/lib/sanitizer_common/sanitizer_platform_limits_posix.cc
compiler-rt/trunk/lib/sanitizer_common/sanitizer_platform_limits_posix.h
Modified: compiler-rt/trunk/lib/asan/lit_tests/TestCases/Linux/interception_readdir_r_test.cc
URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/asan/lit_tests/TestCases/Linux/interception_readdir_r_test.cc?rev=186109&r1=186108&r2=186109&view=diff
==============================================================================
--- compiler-rt/trunk/lib/asan/lit_tests/TestCases/Linux/interception_readdir_r_test.cc (original)
+++ compiler-rt/trunk/lib/asan/lit_tests/TestCases/Linux/interception_readdir_r_test.cc Thu Jul 11 13:51:40 2013
@@ -2,29 +2,56 @@
// RUN: %clangxx_asan -O1 %s -DTEMP_DIR='"'"%T"'"' -o %t && %t 2>&1 | FileCheck %s
// RUN: %clangxx_asan -O2 %s -DTEMP_DIR='"'"%T"'"' -o %t && %t 2>&1 | FileCheck %s
// RUN: %clangxx_asan -O3 %s -DTEMP_DIR='"'"%T"'"' -o %t && %t 2>&1 | FileCheck %s
+//
+// RUN: %clangxx_asan -O0 %s -D_FILE_OFFSET_BITS=64 -DTEMP_DIR='"'"%T"'"' -o %t && %t 2>&1 | FileCheck %s
+// RUN: %clangxx_asan -O1 %s -D_FILE_OFFSET_BITS=64 -DTEMP_DIR='"'"%T"'"' -o %t && %t 2>&1 | FileCheck %s
+// RUN: %clangxx_asan -O2 %s -D_FILE_OFFSET_BITS=64 -DTEMP_DIR='"'"%T"'"' -o %t && %t 2>&1 | FileCheck %s
+// RUN: %clangxx_asan -O3 %s -D_FILE_OFFSET_BITS=64 -DTEMP_DIR='"'"%T"'"' -o %t && %t 2>&1 | FileCheck %s
-#include <stdlib.h>
+#include <dirent.h>
+#include <memory.h>
#include <stdio.h>
+#include <stdlib.h>
#include <unistd.h>
-#include <dirent.h>
int main() {
// Ensure the readdir_r interceptor doesn't erroneously mark the entire dirent
// as written when the end of the directory pointer is reached.
- fputs("reading the " TEMP_DIR " directory...\n", stderr);
+ fputs("test1: reading the " TEMP_DIR " directory...\n", stderr);
DIR *d = opendir(TEMP_DIR);
- struct dirent entry, *result;
+ struct dirent *result = (struct dirent *)(0xfeedbeef);
+ // We assume the temp dir for this test doesn't have crazy long file names.
+ char entry_buffer[4096];
+ memset(entry_buffer, 0xab, sizeof(entry_buffer));
unsigned count = 0;
do {
// Stamp the entry struct to try to trick the interceptor.
- entry.d_reclen = 9999;
- if (readdir_r(d, &entry, &result) != 0)
+ ((struct dirent *)entry_buffer)->d_reclen = 9999;
+ if (readdir_r(d, (struct dirent *)entry_buffer, &result) != 0)
abort();
++count;
} while (result != NULL);
fprintf(stderr, "read %d entries\n", count);
- // CHECK: reading the {{.*}} directory...
+ // CHECK: test1: reading the {{.*}} directory...
+ // CHECK-NOT: stack-buffer-overflow
+ // CHECK: read {{.*}} entries
+
+ // Ensure the readdir64_r interceptor doesn't have the bug either.
+ fputs("test2: reading the " TEMP_DIR " directory...\n", stderr);
+ d = opendir(TEMP_DIR);
+ struct dirent64 *result64;
+ memset(entry_buffer, 0xab, sizeof(entry_buffer));
+ count = 0;
+ do {
+ // Stamp the entry struct to try to trick the interceptor.
+ ((struct dirent64 *)entry_buffer)->d_reclen = 9999;
+ if (readdir64_r(d, (struct dirent64 *)entry_buffer, &result64) != 0)
+ abort();
+ ++count;
+ } while (result64 != NULL);
+ fprintf(stderr, "read %d entries\n", count);
+ // CHECK: test2: reading the {{.*}} directory...
// CHECK-NOT: stack-buffer-overflow
// CHECK: read {{.*}} entries
}
Modified: compiler-rt/trunk/lib/sanitizer_common/sanitizer_common_interceptors.inc
URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_common_interceptors.inc?rev=186109&r1=186108&r2=186109&view=diff
==============================================================================
--- compiler-rt/trunk/lib/sanitizer_common/sanitizer_common_interceptors.inc (original)
+++ compiler-rt/trunk/lib/sanitizer_common/sanitizer_common_interceptors.inc Thu Jul 11 13:51:40 2013
@@ -1372,10 +1372,9 @@ INTERCEPTOR(int, readdir_r, void *dirp,
COMMON_INTERCEPTOR_ENTER(ctx, readdir_r, dirp, entry, result);
int res = REAL(readdir_r)(dirp, entry, result);
if (!res) {
- if (result)
- COMMON_INTERCEPTOR_WRITE_RANGE(ctx, result, sizeof(*result));
- if (entry && *result)
- COMMON_INTERCEPTOR_WRITE_RANGE(ctx, entry, entry->d_reclen);
+ COMMON_INTERCEPTOR_WRITE_RANGE(ctx, result, sizeof(*result));
+ if (*result)
+ COMMON_INTERCEPTOR_WRITE_RANGE(ctx, *result, (*result)->d_reclen);
}
return res;
}
@@ -1403,10 +1402,9 @@ INTERCEPTOR(int, readdir64_r, void *dirp
COMMON_INTERCEPTOR_ENTER(ctx, readdir64_r, dirp, entry, result);
int res = REAL(readdir64_r)(dirp, entry, result);
if (!res) {
- if (result)
- COMMON_INTERCEPTOR_WRITE_RANGE(ctx, result, sizeof(*result));
- if (entry)
- COMMON_INTERCEPTOR_WRITE_RANGE(ctx, entry, entry->d_reclen);
+ COMMON_INTERCEPTOR_WRITE_RANGE(ctx, result, sizeof(*result));
+ if (*result)
+ COMMON_INTERCEPTOR_WRITE_RANGE(ctx, *result, (*result)->d_reclen);
}
return res;
}
@@ -1542,10 +1540,8 @@ INTERCEPTOR(SIZE_T, mbstowcs, wchar_t *d
void *ctx;
COMMON_INTERCEPTOR_ENTER(ctx, mbstowcs, dest, src, len);
SIZE_T res = REAL(mbstowcs)(dest, src, len);
- if (res != (SIZE_T) - 1 && dest) {
- SIZE_T write_cnt = res + (res < len);
- COMMON_INTERCEPTOR_WRITE_RANGE(ctx, dest, write_cnt * sizeof(wchar_t));
- }
+ if (res != (SIZE_T) - 1 && dest)
+ COMMON_INTERCEPTOR_WRITE_RANGE(ctx, dest, (res + 1) * sizeof(wchar_t));
return res;
}
@@ -1553,15 +1549,12 @@ INTERCEPTOR(SIZE_T, mbsrtowcs, wchar_t *
void *ps) {
void *ctx;
COMMON_INTERCEPTOR_ENTER(ctx, mbsrtowcs, dest, src, len, ps);
- if (src) COMMON_INTERCEPTOR_READ_RANGE(ctx, src, sizeof(*src));
- if (ps) COMMON_INTERCEPTOR_READ_RANGE(ctx, ps, mbstate_t_sz);
- SIZE_T res = REAL(mbsrtowcs)(dest, src, len, ps);
- if (res != (SIZE_T)(-1) && dest && src) {
- // This function, and several others, may or may not write the terminating
- // \0 character. They write it iff they clear *src.
- SIZE_T write_cnt = res + !*src;
- COMMON_INTERCEPTOR_WRITE_RANGE(ctx, dest, write_cnt * sizeof(wchar_t));
+ if (src) {
+ COMMON_INTERCEPTOR_READ_RANGE(ctx, src, sizeof(*src));
}
+ SIZE_T res = REAL(mbsrtowcs)(dest, src, len, ps);
+ if (res != (SIZE_T) - 1 && dest)
+ COMMON_INTERCEPTOR_WRITE_RANGE(ctx, dest, (res + 1) * sizeof(wchar_t));
return res;
}
@@ -1581,12 +1574,9 @@ INTERCEPTOR(SIZE_T, mbsnrtowcs, wchar_t
COMMON_INTERCEPTOR_READ_RANGE(ctx, src, sizeof(*src));
if (nms) COMMON_INTERCEPTOR_READ_RANGE(ctx, *src, nms);
}
- if (ps) COMMON_INTERCEPTOR_READ_RANGE(ctx, ps, mbstate_t_sz);
SIZE_T res = REAL(mbsnrtowcs)(dest, src, nms, len, ps);
- if (res != (SIZE_T)(-1) && dest && src) {
- SIZE_T write_cnt = res + !*src;
- COMMON_INTERCEPTOR_WRITE_RANGE(ctx, dest, write_cnt * sizeof(wchar_t));
- }
+ if (res != (SIZE_T) - 1 && dest)
+ COMMON_INTERCEPTOR_WRITE_RANGE(ctx, dest, (res + 1) * sizeof(wchar_t));
return res;
}
@@ -1600,10 +1590,8 @@ INTERCEPTOR(SIZE_T, wcstombs, char *dest
void *ctx;
COMMON_INTERCEPTOR_ENTER(ctx, wcstombs, dest, src, len);
SIZE_T res = REAL(wcstombs)(dest, src, len);
- if (res != (SIZE_T) - 1 && dest) {
- SIZE_T write_cnt = res + (res < len);
- COMMON_INTERCEPTOR_WRITE_RANGE(ctx, dest, write_cnt);
- }
+ if (res != (SIZE_T) - 1 && dest)
+ COMMON_INTERCEPTOR_WRITE_RANGE(ctx, dest, res + 1);
return res;
}
@@ -1611,13 +1599,12 @@ INTERCEPTOR(SIZE_T, wcsrtombs, char *des
void *ps) {
void *ctx;
COMMON_INTERCEPTOR_ENTER(ctx, wcsrtombs, dest, src, len, ps);
- if (src) COMMON_INTERCEPTOR_READ_RANGE(ctx, src, sizeof(*src));
- if (ps) COMMON_INTERCEPTOR_READ_RANGE(ctx, ps, mbstate_t_sz);
- SIZE_T res = REAL(wcsrtombs)(dest, src, len, ps);
- if (res != (SIZE_T) - 1 && dest && src) {
- SIZE_T write_cnt = res + !*src;
- COMMON_INTERCEPTOR_WRITE_RANGE(ctx, dest, write_cnt);
+ if (src) {
+ COMMON_INTERCEPTOR_READ_RANGE(ctx, src, sizeof(*src));
}
+ SIZE_T res = REAL(wcsrtombs)(dest, src, len, ps);
+ if (res != (SIZE_T) - 1 && dest)
+ COMMON_INTERCEPTOR_WRITE_RANGE(ctx, dest, res + 1);
return res;
}
@@ -1637,12 +1624,9 @@ INTERCEPTOR(SIZE_T, wcsnrtombs, char *de
COMMON_INTERCEPTOR_READ_RANGE(ctx, src, sizeof(*src));
if (nms) COMMON_INTERCEPTOR_READ_RANGE(ctx, *src, nms);
}
- if (ps) COMMON_INTERCEPTOR_READ_RANGE(ctx, ps, mbstate_t_sz);
SIZE_T res = REAL(wcsnrtombs)(dest, src, nms, len, ps);
- if (res != (SIZE_T) - 1 && dest && src) {
- SIZE_T write_cnt = res + !*src;
- COMMON_INTERCEPTOR_WRITE_RANGE(ctx, dest, write_cnt);
- }
+ if (res != (SIZE_T) - 1 && dest)
+ COMMON_INTERCEPTOR_WRITE_RANGE(ctx, dest, res + 1);
return res;
}
Modified: compiler-rt/trunk/lib/sanitizer_common/sanitizer_platform_limits_posix.cc
URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_platform_limits_posix.cc?rev=186109&r1=186108&r2=186109&view=diff
==============================================================================
--- compiler-rt/trunk/lib/sanitizer_common/sanitizer_platform_limits_posix.cc (original)
+++ compiler-rt/trunk/lib/sanitizer_common/sanitizer_platform_limits_posix.cc Thu Jul 11 13:51:40 2013
@@ -795,12 +795,20 @@ CHECK_SIZE_AND_OFFSET(cmsghdr, cmsg_len)
CHECK_SIZE_AND_OFFSET(cmsghdr, cmsg_level);
CHECK_SIZE_AND_OFFSET(cmsghdr, cmsg_type);
+COMPILER_CHECK(sizeof(__sanitizer_dirent) <= sizeof(dirent));
CHECK_SIZE_AND_OFFSET(dirent, d_ino);
#ifndef SANITIZER_MAC
CHECK_SIZE_AND_OFFSET(dirent, d_off);
#endif
CHECK_SIZE_AND_OFFSET(dirent, d_reclen);
+#if SANITIZER_LINUX && !SANITIZER_ANDROID
+COMPILER_CHECK(sizeof(__sanitizer_dirent64) <= sizeof(dirent64));
+CHECK_SIZE_AND_OFFSET(dirent64, d_ino);
+CHECK_SIZE_AND_OFFSET(dirent64, d_off);
+CHECK_SIZE_AND_OFFSET(dirent64, d_reclen);
+#endif
+
CHECK_TYPE_SIZE(ifconf);
CHECK_SIZE_AND_OFFSET(ifconf, ifc_len);
CHECK_SIZE_AND_OFFSET(ifconf, ifc_ifcu);
Modified: compiler-rt/trunk/lib/sanitizer_common/sanitizer_platform_limits_posix.h
URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_platform_limits_posix.h?rev=186109&r1=186108&r2=186109&view=diff
==============================================================================
--- compiler-rt/trunk/lib/sanitizer_common/sanitizer_platform_limits_posix.h (original)
+++ compiler-rt/trunk/lib/sanitizer_common/sanitizer_platform_limits_posix.h Thu Jul 11 13:51:40 2013
@@ -118,8 +118,8 @@ namespace __sanitizer {
#if SANITIZER_LINUX && !SANITIZER_ANDROID
struct __sanitizer_dirent64 {
- uptr d_ino;
- uptr d_off;
+ unsigned long long d_ino;
+ unsigned long long d_off;
unsigned short d_reclen;
// more fields that we don't care about
};
More information about the llvm-commits
mailing list