[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:21 PDT 2013
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/b43b1337/attachment.html>
More information about the llvm-commits
mailing list