<div dir="ltr"><div>Hi Chandler!</div><div><br></div><div>First of all - thanks for finding and cleaning this up (and sorry for the breakage/bugs)!</div><div>Some comments inline.</div><br><div class="gmail_extra"><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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style: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></blockquote><div><br></div><div>I agree that NULL "entry" shouldn't be considered as a valid input, but...</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<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></blockquote><div><br></div><div>Here's what I get from "man readdir_r":</div><div><br></div><div>It reads the next directory entry from the directory stream dirp, and returns it in the caller-allocated buffer pointed to by entry.<br>
</div><div>So this _looks like_ we may assume that *entry is indeed overwritten in the original function.</div><div>That said, I'm ok with the changes you've made.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<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>
}</blockquote><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
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></blockquote><div><br></div><div>Why? Manual for mbstowcs and friends explicitly tells that trailing '\0' is *not* always appended</div>
<div>(I've verified this by running these functions manually), so we actually have to consider different cases here,</div><div>as the original code does. I went on and reverted these changes in r186155.</div><div><br>
</div><div>Your change also broke our buildbots and MemorySanitizer tests ("make check-msan"). It is possible that</div><div>you haven't noticed this because MSan unit tests are not built in your build tree - most likely you don't</div>
<div>have "projects/libcxx" checked out - it is required to build MSan unittests.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
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></blockquote><div><br></div><div>Why did you remove checking the shadow for ps? If you rey to be more conservative</div><div>(for example, assume that we don't know how ps is modified, so there's no reason in</div>
<div>checking it), then we should probably add</div><div>COMMON_INTERCEPTOR_WRITE_RANGE instead (as man says *ps is updated in certain cases).</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
- 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></blockquote><div><br></div><div>I've removed "if (src)" checks here, as NULL src doesn't make any sense, and the original</div><div>function segfaults if src is NULL. But...</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+ 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></blockquote><div><br></div><div>we may be conservative and allow src to be NULL if nms is 0.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
- 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></blockquote><div><br></div><div>I assume, you found this while testing 32-bit runtime?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
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">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 clear="all"><div><br></div>-- <br><div>Alexey Samsonov, MSK</div>
</div></div>