[compiler-rt] r186109 - Fix a veritable conucopia of bugs in the readdir_r interceptors.

Alexey Samsonov samsonov at google.com
Fri Jul 12 05:12:11 PDT 2013


Hi Chandler!

First of all - thanks for finding and cleaning this up (and sorry for the
breakage/bugs)!
Some comments inline.

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.
>

I agree that NULL "entry" shouldn't be considered as a valid input, but...


>
> 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.
>

Here's what I get from "man readdir_r":

It reads the next directory entry from the directory stream dirp, and
returns it in the caller-allocated buffer pointed to by entry.
So this _looks like_ we may assume that *entry is indeed overwritten in the
original function.
That said, I'm ok with the changes you've made.


>
> 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));
>

Why? Manual for mbstowcs and friends explicitly tells that trailing '\0' is
*not* always appended
(I've verified this by running these functions manually), so we actually
have to consider different cases here,
as the original code does. I went on and reverted these changes in r186155.

Your change also broke our buildbots and MemorySanitizer tests ("make
check-msan"). It is possible that
you haven't noticed this because MSan unit tests are not built in your
build tree - most likely you don't
have "projects/libcxx" checked out - it is required to build MSan unittests.



>    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);
>

Why did you remove checking the shadow for ps? If you rey to be more
conservative
(for example, assume that we don't know how ps is modified, so there's no
reason in
checking it), then we should probably add
COMMON_INTERCEPTOR_WRITE_RANGE instead (as man says *ps is updated in
certain cases).


> -  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));
>    }
>

I've removed "if (src)" checks here, as NULL src doesn't make any sense,
and the original
function segfaults if src is NULL. But...


> +  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);
>    }
>

we may be conservative and allow src to be NULL if nms is 0.


> -  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;
>

I assume, you found this while testing 32-bit runtime?


>      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
>


-- 
Alexey Samsonov, MSK
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130712/43b05bcd/attachment.html>


More information about the llvm-commits mailing list