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