[compiler-rt] r186109 - Fix a veritable conucopia of bugs in the readdir_r interceptors.
Sergey Matveev
earthdok at google.com
Thu Jul 11 12:02:43 PDT 2013
s/dirent/struct dirent/
On Thu, Jul 11, 2013 at 11:02 PM, Sergey Matveev <earthdok at google.com>wrote:
> 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.
>
>
> On Thu, Jul 11, 2013 at 10:51 PM, Chandler Carruth <chandlerc at gmail.com>wrote:
>
>> 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
>> };
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130711/ae304387/attachment.html>
More information about the llvm-commits
mailing list