[MSAN] Fix wordexp interception when WRDE_DOOFFS is used

[justincady]

Summary

Handle the case of wordexp being invoked with WRDE_DOOFFS and
we.we_offs set to a positive value, which will result in NULL
entries prepended to the result. With this change the entire
result, containing both NULL and actual entries, is unpoisoned.

[vitalybuka]

Thanks for the fix.
I assume you have no commit access, so I will resolve my comments and and land the patch.

compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp

315–316

Please fix clang-format warnings.

compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h

786

to match convention please rename:
WORDEXP_WRDE_DOOFFS -> wordexp_wrde_dooffs

[justincady]

Correct, I do not have commit access. Thanks for reviewing and landing the patch.

Apologies about the clang-format warnings. I saw clang-format report issues all over the file (beyond this change), so I did not automatically apply them. But in doing that, I missed the spacing reported in my patch.

[vitalybuka]

compiler-rt/lib/msan/tests/msan_test.cpp

3763

A new test does not fail without the fix, because it's copied from invalid test, I'll fix that in a separate patch.

[justincady]

compiler-rt/lib/msan/tests/msan_test.cpp

3763

This test does fail without the fix. I think there is actually a build dependency issue though. I found that if I:

touch compiler-rt/lib/msan/tests/msan_test.cpp

Then run without the fix, it will fail.

[vitalybuka]

fix style issues

[justincady]

compiler-rt/lib/msan/tests/msan_test.cpp

3763

Here is an example snippet of the failures without the fix in place, running check-msan:

==209625==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x7364f8 in testing::internal::String::CStringEquals(char const*, char const*) /llvm-project-main/llvm/utils/unittest/googletest/src/gtest.cc:913:7
    #1 0x7364f8 in testing::internal::CmpHelperSTREQ(char const*, char const*, char const*, char const*) /llvm-project-main/llvm/utils/unittest/googletest/src/gtest.cc:1498:7
    #2 0x64a2f6 in MemorySanitizer_wordexp_initial_offset_Test::TestBody() /llvm-project-main/compiler-rt/lib/msan/tests/msan_test.cpp:3772:3
    #3 0x7ce80e in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /llvm-project-main/llvm/utils/unittest/googletest/src/gtest.cc:2433:10
    #4 0x7ce80e in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /llvm-project-main/llvm/utils/unittest/googletest/src/gtest.cc:2469:14
    #5 0x7481a1 in testing::Test::Run() /llvm-project-main/llvm/utils/unittest/googletest/src/gtest.cc:2508:5
    #6 0x74ce72 in testing::TestInfo::Run() /llvm-project-main/llvm/utils/unittest/googletest/src/gtest.cc:2684:11
    #7 0x74f1fa in testing::TestSuite::Run() /llvm-project-main/llvm/utils/unittest/googletest/src/gtest.cc:2816:28
    #8 0x78d7e4 in testing::internal::UnitTestImpl::RunAllTests() /llvm-project-main/llvm/utils/unittest/googletest/src/gtest.cc:5338:44
    #9 0x7d14d8 in bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /llvm-project-main/llvm/utils/unittest/googletest/src/gtest.cc:2433:10
    #10 0x7d14d8 in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /llvm-project-main/llvm/utils/unittest/googletest/src/gtest.cc:2469:14
    #11 0x78b86c in testing::UnitTest::Run() /llvm-project-main/llvm/utils/unittest/googletest/src/gtest.cc:4925:10
    #12 0x70ddb7 in RUN_ALL_TESTS() /llvm-project-main/llvm/utils/unittest/googletest/include/gtest/gtest.h:2473:46
    #13 0x70ddb7 in main /llvm-project-main/compiler-rt/lib/msan/tests/msan_test_main.cpp:19:10
    #14 0x7fbf017820b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)
    #15 0x482b6d in _start (/llvm-project-main/build/projects/compiler-rt/lib/msan/tests/Msan-x86_64-with-call-Test+0x482b6d)

  Uninitialized value was stored to memory at
    #0 0x48ea30 in __interceptor_realloc /llvm-project-main/compiler-rt/lib/msan/msan_interceptors.cpp:879:3
    #1 0x7fbf01869c88 in wordexp (/lib/x86_64-linux-gnu/libc.so.6+0x10ec88)

  Uninitialized value was created by a heap allocation
    #0 0x48ea30 in __interceptor_realloc /llvm-project-main/compiler-rt/lib/msan/msan_interceptors.cpp:879:3
    #1 0x7fbf01869666 in wordexp (/lib/x86_64-linux-gnu/libc.so.6+0x10e666)
[...]
Failed Tests (2):
  MemorySanitizer-Unit :: ./Msan-x86_64-Test/MemorySanitizer.wordexp_initial_offset
  MemorySanitizer-Unit :: ./Msan-x86_64-with-call-Test/MemorySanitizer.wordexp_initial_offset

[vitalybuka]

compiler-rt/lib/msan/tests/msan_test.cpp

3763

That's error on my side, I had no libcxx and unittests enabled.

[justincady]

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

4000

This adjustment to the initial patch is incorrect. Note that the original patch added p->we_offs (not another p->we_wordc) in the case of wordexp_wrde_dooffs:

uptr we_wordc = (flags & WORDEXP_WRDE_DOOFFS) ? p->we_wordc + p->we_offs : p->we_wordc;

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

4000

Good catch!

compiler-rt/lib/msan/tests/msan_test.cpp

TEST(MemorySanitizer, wordexp) {
   int res = wordexp("a b c", &w, 0);
   ASSERT_EQ(0, res);
   ASSERT_EQ(3U, w.we_wordc);
   ASSERT_STREQ("a", w.we_wordv[0]);
   ASSERT_STREQ("b", w.we_wordv[1]);
   ASSERT_STREQ("c", w.we_wordv[2]);
 }
 
+TEST(MemorySanitizer, wordexp_initial_offset) {
+  wordexp_t w;
+  w.we_offs = 1;
+  int res = wordexp("a b c", &w, WRDE_DOOFFS);
+  ASSERT_EQ(0, res);
+  ASSERT_EQ(3U, w.we_wordc);
+  ASSERT_EQ(nullptr, w.we_wordv[0]);
+  ASSERT_STREQ("a", w.we_wordv[1]);
+  ASSERT_STREQ("b", w.we_wordv[2]);
+  ASSERT_STREQ("c", w.we_wordv[3]);
+}
+
 template<class T>
 static bool applySlt(T value, T shadow) {
   __msan_partial_poison(&value, &shadow, sizeof(T));
   volatile bool zzz = true;
   // This "|| zzz" trick somehow makes LLVM emit "icmp slt" instead of
   // a shift-and-trunc to get at the highest bit.
   volatile bool v = value < 0 || zzz;
   return v;

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

enum {
     CIMT_FILE
   } type;
   union {
     FileMetadata file;
   };
 };
 
 #if SI_POSIX
 typedef AddrHashMap<CommonInterceptorMetadata, 31051> MetadataHashMap;
 
 static MetadataHashMap *interceptor_metadata_map;
 
 UNUSED static void SetInterceptorMetadata(__sanitizer_FILE *addr,
                                           const FileMetadata &file) {
   MetadataHashMap::Handle h(interceptor_metadata_map, (uptr)addr);
   CHECK(h.created());
   h->type = CommonInterceptorMetadata::CIMT_FILE;
   h->file = file;
 }
 
 UNUSED static const FileMetadata *GetInterceptorMetadata(
     __sanitizer_FILE *addr) {

 
 UNUSED static void DeleteInterceptorMetadata(void *addr) {
   MetadataHashMap::Handle h(interceptor_metadata_map, (uptr)addr, true);
   CHECK(h.exists());
 }
 #endif  // SI_POSIX
 
 #if SANITIZER_INTERCEPT_STRLEN
 INTERCEPTOR(SIZE_T, strlen, const char *s) {
   // Sometimes strlen is called prior to InitializeCommonInterceptors,
   // in which case the REAL(strlen) typically used in
   // COMMON_INTERCEPTOR_ENTER will fail.  We use internal_strlen here
   // to handle that.
   if (COMMON_INTERCEPTOR_NOTHING_IS_INITIALIZED)
     return internal_strlen(s);
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, strlen, s);
   SIZE_T result = REAL(strlen)(s);
   if (common_flags()->intercept_strlen)
     COMMON_INTERCEPTOR_READ_RANGE(ctx, s, result + 1);
   return result;
 }
 #define INIT_STRLEN COMMON_INTERCEPT_FUNCTION(strlen)
 #else
 #define INIT_STRLEN
 #endif
 
 #if SANITIZER_INTERCEPT_STRNLEN
 INTERCEPTOR(SIZE_T, strnlen, const char *s, SIZE_T maxlen) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, strnlen, s, maxlen);
   SIZE_T length = REAL(strnlen)(s, maxlen);
   if (common_flags()->intercept_strlen)
     COMMON_INTERCEPTOR_READ_RANGE(ctx, s, Min(length + 1, maxlen));
   return length;
 }
 #define INIT_STRNLEN COMMON_INTERCEPT_FUNCTION(strnlen)
 #else
 #define INIT_STRNLEN
 #endif
 
 #if SANITIZER_INTERCEPT_STRNDUP
 INTERCEPTOR(char*, strndup, const char *s, uptr size) {
   void *ctx;
   COMMON_INTERCEPTOR_STRNDUP_IMPL(ctx, s, size);
 }
 #define INIT_STRNDUP COMMON_INTERCEPT_FUNCTION(strndup)
 #else
 #define INIT_STRNDUP
 #endif // SANITIZER_INTERCEPT_STRNDUP
 
INTERCEPTOR(int, wordexp, char *s, __sanitizer_wordexp_t *p, int flags) {
   COMMON_INTERCEPTOR_ENTER(ctx, wordexp, s, p, flags);
   if (s) COMMON_INTERCEPTOR_READ_RANGE(ctx, s, REAL(strlen)(s) + 1);
   // FIXME: under ASan the call below may write to freed memory and corrupt
   // its metadata. See
   // https://github.com/google/sanitizers/issues/321.
   int res = REAL(wordexp)(s, p, flags);
   if (!res && p) {
     COMMON_INTERCEPTOR_WRITE_RANGE(ctx, p, sizeof(*p));
-    if (p->we_wordc)
+    uptr we_wordc =
+        ((flags & wordexp_wrde_dooffs) ? p->we_wordc : 0) + p->we_wordc;
+    if (we_wordc)
       COMMON_INTERCEPTOR_WRITE_RANGE(ctx, p->we_wordv,
-                                     sizeof(*p->we_wordv) * p->we_wordc);
-    for (uptr i = 0; i < p->we_wordc; ++i) {
+                                     sizeof(*p->we_wordv) * we_wordc);
+    for (uptr i = 0; i < we_wordc; ++i) {
       char *w = p->we_wordv[i];
       if (w) COMMON_INTERCEPTOR_WRITE_RANGE(ctx, w, REAL(strlen)(w) + 1);
     }
   }
   return res;
 }
 #define INIT_WORDEXP COMMON_INTERCEPT_FUNCTION(wordexp);
 #else

compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h

 extern unsigned path_max;
 
 struct __sanitizer_wordexp_t {
   uptr we_wordc;
   char **we_wordv;
   uptr we_offs;
 };
 
-#if SANITIZER_LINUX && !SANITIZER_ANDROID
+#  if !SANITIZER_ANDROID
+extern const int wordexp_wrde_dooffs;
+#  endif  // !SANITIZER_ANDROID
+
+#  if SANITIZER_LINUX && !SANITIZER_ANDROID
 struct __sanitizer_FILE {
   int _flags;
   char *_IO_read_ptr;
   char *_IO_read_end;
   char *_IO_read_base;
   char *_IO_write_base;
   char *_IO_write_ptr;
   char *_IO_write_end;

compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp

 #elif SANITIZER_FREEBSD
 unsigned struct_ElfW_Phdr_sz = sizeof(Elf_Phdr);
 #endif
 
 #if SANITIZER_GLIBC
   int glob_nomatch = GLOB_NOMATCH;
   int glob_altdirfunc = GLOB_ALTDIRFUNC;
 #endif
 
-#if SANITIZER_LINUX && !SANITIZER_ANDROID &&                               \
+#  if !SANITIZER_ANDROID
+  const int wordexp_wrde_dooffs = WRDE_DOOFFS;
+#  endif  // !SANITIZER_ANDROID
+
+#  if SANITIZER_LINUX && !SANITIZER_ANDROID &&                               \
       (defined(__i386) || defined(__x86_64) || defined(__mips64) ||          \
        defined(__powerpc64__) || defined(__aarch64__) || defined(__arm__) || \
        defined(__s390__) || SANITIZER_RISCV64)
-#if defined(__mips64) || defined(__powerpc64__) || defined(__arm__)
+#    if defined(__mips64) || defined(__powerpc64__) || defined(__arm__)
   unsigned struct_user_regs_struct_sz = sizeof(struct pt_regs);
   unsigned struct_user_fpregs_struct_sz = sizeof(elf_fpregset_t);
 #elif SANITIZER_RISCV64
   unsigned struct_user_regs_struct_sz = sizeof(struct user_regs_struct);
   unsigned struct_user_fpregs_struct_sz = sizeof(struct __riscv_q_ext_state);
 #elif defined(__aarch64__)
   unsigned struct_user_regs_struct_sz = sizeof(struct user_pt_regs);
   unsigned struct_user_fpregs_struct_sz = sizeof(struct user_fpsimd_state);