[msan] Intercept dladdr1, and refactor dladdr

[thurston]

Summary

This patch adds an msan interceptor for dladdr1 (with support for RTLD_DL_LINKMAP and RTLD_DL_SYMENT) and an accompanying test. It also adds a helper file, msan_dl.cpp, that contains UnpoisonDllAddrInfo (refactored out of the dladdr interceptor) and UnpoisonDllAddr1ExtraInfo.

[vitalybuka]

compiler-rt/lib/msan/msan_interceptors.cpp

1537–1553

what if we move these into a separate cpp file and include headers with them
same for link_map
we don't want header in the interceptor file, other cpp should work

And add most of interceptor body into UnpoisonDllAddr1ExtraInfo()

1537–1553

e.g. msan_dl.cpp

compiler-rt/test/msan/dladdr1_test.c

4

do you need FileCheck here, or asserts is enough?

[vitalybuka]

compiler-rt/lib/msan/msan_interceptors.cpp

1537–1553

same struct dlinfo

[thurston]

compiler-rt/lib/msan/msan_interceptors.cpp

1537–1553

Good idea, I will make those changes and update this CL.

compiler-rt/test/msan/dladdr1_test.c

4

// CHECK: libc.so
// CHECK: dladdr1_test

are inconvenient to implement as asserts, because they're not exact matches. The output on my machine is:

0x563819032000: '', 0x563819035dc8
0x7ffe4b330000: 'linux-vdso.so.1', 0x7ffe4b3303e0
0x7fb9237d7000: '/lib/x86_64-linux-gnu/libc.so.6', 0x7fb9239a8b60
0x7fb9239d2000: '/lib64/ld-linux-x86-64.so.2', 0x7fb923a03e20
0x7fb9239cb000: '/tmp/dl-so.so', 0x7fb9239cee20

but I expect subtle differences on build bots and other machines that aren't precisely the same 64-bit Linux.

[thurston]

compiler-rt/test/msan/dladdr1_test.c

4

Oops, that last line of the output should be

0x7f96e35b6000: '/usr/local/google/home/thurston/llvm-projectD/build/projects/compiler-rt/test/msan/lld-X86_64/Output/dladdr1_test.c.tmp-so.so', 0x7f96e35b86a8

(the previous output was from my working copy in /tmp)

[thurston]

Adds a new header file, msan_dl.h, per Vitaly's suggestion.

[vitalybuka]

compiler-rt/lib/msan/msan_dl.h

13

that's not what I suggested

suggestion is to not define these structs but:
#include <dlfcn.h> and friends in a cpp file
and export function like UnpoisonDllAddr1ExtraInfo

[thurston]

Refactor into UnpoisonDllAddr1ExtraInfo(), per Vitaly's suggestion

[thurston]

compiler-rt/lib/msan/msan_dl.h

13

Ah, I see. Let me fix that.

[thurston]

"#include <dlfcn.h> and friends in a cpp file and export function like UnpoisonDllAddr1ExtraInfo"

[vitalybuka]

compiler-rt/lib/msan/msan_dl.cpp

2

please add standard file headers

5

#include "msan_dl.h"

40

Same for void UnpoisonDllAddrInfo?

compiler-rt/lib/msan/msan_interceptors.cpp

20

.h is not in the patch

[vitalybuka]

compiler-rt/lib/msan/msan_dl.cpp

22–28

[thurston]

compiler-rt/lib/msan/msan_interceptors.cpp

1537–1553

If I #include dlfcn.h to get the definition of struct Dl_info, it will also drag in the declaration of dladdr/dladdr1/dlerror, conflicting with the interceptors. I suspect that's why msan_interceptors.cpp had defined their own struct dlinfo.

[vitalybuka]

compiler-rt/lib/msan/msan_interceptors.cpp

1537–1553

you will include dlfcn.h only into msan_dl.cpp

[thurston]

Refactor

[thurston]

Change order of headers

[vitalybuka]

compiler-rt/lib/msan/msan_dl.cpp

40

can map be nullptr? we may crash on l_prev then

[thurston]

compiler-rt/lib/msan/msan_interceptors.cpp

1537–1553

I ended up also having to replace Dl_info* with void*, in everywhere except msan_dl.cpp.

[vitalybuka]

compiler-rt/lib/msan/msan_dl.cpp

2

still no header?

//===-- msan_interceptors.cpp ---------------------------------------------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//

compiler-rt/lib/msan/msan_dl.h

2

also

//===-- msan_interceptors.cpp ---------------------------------------------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//

[thurston]

compiler-rt/lib/msan/msan_dl.cpp

2

Sorry, I misinterpreted "add standard file headers" as "add #include <x.h>". I'll fix.

[vitalybuka]

compiler-rt/lib/msan/msan_dl.cpp

2–5

[thurston]

Fix null pointer case
Add license to top of header files

[vitalybuka]

compiler-rt/lib/msan/msan_dl.cpp

43–52

[thurston]

Made 'if (! map) return;' change

[thurston]

Fix 'cast from 'void ' to 'const Elf64_Sym ' must have all intermediate pointers const qualified to be safe [-Werror,-Wcast-qual]'

[brooks]

This change broke the build of msan on FreeBSD. dladdr1 seems to be a glibc-ism and is not present in FreeBSD or musl. All this functionality needs to be gated by a CMake check.

[thurston]

In D154272#4479087, @brooks wrote:

This change broke the build of msan on FreeBSD. dladdr1 seems to be a glibc-ism and is not present in FreeBSD or musl. All this functionality needs to be gated by a CMake check.

@brooks Thanks for the heads up, and apologies for the inconvenience. To fix the build, I've reverted my patch (https://reviews.llvm.org/rG015dabd7672f936cdb5bdcad20fe80b17f05c9ca) and will gate it appropriately before I reland it.

[thurston]

Add '#if SANITIZER_GLIBC' guards to avoid breaking FreeBSD

compiler-rt/lib/msan/CMakeLists.txt

 include_directories(..)
 
 # Runtime library sources and build flags.
 set(MSAN_RTL_SOURCES
   msan.cpp
   msan_allocator.cpp
   msan_chained_origin_depot.cpp
+  msan_dl.cpp
   msan_interceptors.cpp
   msan_linux.cpp
   msan_report.cpp
   msan_thread.cpp
   msan_poisoning.cpp
   )
 
 set(MSAN_RTL_CXX_SOURCES
   msan_new_delete.cpp
   )
 
 set(MSAN_RTL_HEADERS
   msan.h
   msan_allocator.h
   msan_chained_origin_depot.h
+  msan_dl.h
   msan_flags.h
   msan_flags.inc
   msan_interface_internal.h
   msan_origin.h
   msan_poisoning.h
   msan_report.h
   msan_thread.h
   )

compiler-rt/lib/msan/msan_dl.h

+#ifndef MSAN_DL_H
+#define MSAN_DL_H
+
+#include "msan.h"
+#include "sanitizer_common/sanitizer_common.h"
+
+namespace __msan {
+
+void UnpoisonDllAddr1ExtraInfo(void **extra_info, int flags);
+
+struct dlinfo {
+  char *dli_fname;
+  void *dli_fbase;
+  char *dli_sname;
+  void *dli_saddr;
+};
+
+#if SANITIZER_WORDSIZE == 32
+struct sym {
+  u32 st_name;
+  u32 st_value;
+  u32 st_size;
+  unsigned char st_info;
+  unsigned char st_other;
+  u16 st_shndx;
+};
+#elif SANITIZER_WORDSIZE == 64
+struct sym {
+  u32 st_name;
+  unsigned char st_info;
+  unsigned char st_other;
+  u16 st_shndx;
+  u64 st_value;
+  u64 st_size;
+};
+#else
+#  error "Unexpected SANITIZER_WORDSIZE"
+#endif
+
+// 'sanitizer_common/sanitizer_linux.h' defines 'struct link_map'
+// as an opaque type.
+struct link_map_internals {
+  unsigned long l_addr;
+  char *l_name;
+  unsigned long l_ld;
+  struct link_map_internals *l_next, *l_prev;
+};
+
+}  // namespace __msan
+
+#endif  // MSAN_DL_H

compiler-rt/lib/msan/msan_dl.cpp

+#include "msan_dl.h"
+
+#include "msan_poisoning.h"
+
+namespace __msan {
+
+void UnpoisonDllAddr1ExtraInfo(void **extra_info, int flags) {
+  if (flags == 1) {  // RTLD_DL_SYMENT
+    __msan_unpoison(extra_info, sizeof(void *));
+
+    struct sym *s = *((struct sym **)(extra_info));
+    __msan_unpoison(s, sizeof(struct sym));
+  } else if (flags == 2) {  // RTLD_DL_LINKMAP
+    __msan_unpoison(extra_info, sizeof(void *));
+
+    struct link_map_internals *map =
+        *((struct link_map_internals **)(extra_info));
+
+    // Walk forward
+    struct link_map_internals *ptr = map;
+    while (ptr) {
+      __msan_unpoison(ptr, sizeof(struct link_map_internals));
+      if (ptr->l_name)
+        __msan_unpoison(ptr->l_name, internal_strlen(ptr->l_name) + 1);
+      ptr = ptr->l_next;
+    }
+
+    // Walk backward
+    ptr = map->l_prev;
+    while (ptr) {
+      __msan_unpoison(ptr, sizeof(struct link_map_internals));
+      if (ptr->l_name)
+        __msan_unpoison(ptr->l_name, internal_strlen(ptr->l_name) + 1);
+      ptr = ptr->l_prev;
+    }
+  }
+}
+
+}  // namespace __msan

compiler-rt/lib/msan/msan_interceptors.cpp

 //
 // FIXME: move as many interceptors as possible into
 // sanitizer_common/sanitizer_common_interceptors.h
 //===----------------------------------------------------------------------===//
 
 #include "interception/interception.h"
 #include "msan.h"
 #include "msan_chained_origin_depot.h"
+#include "msan_dl.h"
 #include "msan_origin.h"
 #include "msan_poisoning.h"
 #include "msan_report.h"
 #include "msan_thread.h"
 #include "sanitizer_common/sanitizer_allocator.h"
 #include "sanitizer_common/sanitizer_allocator_dlsym.h"
 #include "sanitizer_common/sanitizer_allocator_interface.h"
 #include "sanitizer_common/sanitizer_atomic.h"
INTERCEPTOR(const char *, strsignal, int sig) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, strsignal, sig);
   const char *res = REAL(strsignal)(sig);
   if (res)
     __msan_unpoison(res, internal_strlen(res) + 1);
   return res;
 }
 
-struct dlinfo {
-  char *dli_fname;
-  void *dli_fbase;
-  char *dli_sname;
-  void *dli_saddr;
-};
-
 INTERCEPTOR(int, dladdr, void *addr, dlinfo *info) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, dladdr, addr, info);
   int res = REAL(dladdr)(addr, info);
   if (res != 0) {
     __msan_unpoison(info, sizeof(*info));
     if (info->dli_fname)
       __msan_unpoison(info->dli_fname, internal_strlen(info->dli_fname) + 1);
     if (info->dli_sname)
       __msan_unpoison(info->dli_sname, internal_strlen(info->dli_sname) + 1);
   }
   return res;
 }
 
+INTERCEPTOR(int, dladdr1, void *addr, dlinfo *info, void **extra_info,
+            int flags) {
+  void *ctx;
+  COMMON_INTERCEPTOR_ENTER(ctx, dladdr1, addr, info, extra_info, flags);
+  int res = REAL(dladdr1)(addr, info, extra_info, flags);
+  if (res != 0) {
+    __msan_unpoison(info, sizeof(*info));
+    if (info->dli_fname)
+      __msan_unpoison(info->dli_fname, internal_strlen(info->dli_fname) + 1);
+    if (info->dli_sname)
+      __msan_unpoison(info->dli_sname, internal_strlen(info->dli_sname) + 1);
+
+    UnpoisonDllAddr1ExtraInfo(extra_info, flags);
+  }
+  return res;
+}
+
 INTERCEPTOR(char *, dlerror, int fake) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, dlerror, fake);
   char *res = REAL(dlerror)(fake);
   if (res)
     __msan_unpoison(res, internal_strlen(res) + 1);
   return res;
 }
 
#endif
   MSAN_MAYBE_INTERCEPT_GETRLIMIT64;
   MSAN_MAYBE_INTERCEPT_PRLIMIT;
   MSAN_MAYBE_INTERCEPT_PRLIMIT64;
   INTERCEPT_FUNCTION(gethostname);
   MSAN_MAYBE_INTERCEPT_EPOLL_WAIT;
   MSAN_MAYBE_INTERCEPT_EPOLL_PWAIT;
   INTERCEPT_FUNCTION(strsignal);
   INTERCEPT_FUNCTION(dladdr);
+  INTERCEPT_FUNCTION(dladdr1);
   INTERCEPT_FUNCTION(dlerror);
   INTERCEPT_FUNCTION(dl_iterate_phdr);
   INTERCEPT_FUNCTION(getrusage);
 #if defined(__mips__)
   INTERCEPT_FUNCTION_VER(pthread_create, "GLIBC_2.2");
 #else
   INTERCEPT_FUNCTION(pthread_create);
 #endif

compiler-rt/test/msan/dladdr1_test.c

+/* RUN: %clang_msan -g %s -o %t
+   RUN: %clang_msan -g %s -DBUILD_SO -fPIC -o %t-so.so -shared
+   RUN: %run %t 2>&1 | FileCheck %s
+
+   REQUIRES: glibc{{.*}}
+*/
+
+#define _GNU_SOURCE
+
+#ifndef BUILD_SO
+#include <assert.h>
+#include <dlfcn.h>
+#include <elf.h>
+#include <link.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+typedef volatile long *(* get_t)();
+get_t GetTls;
+
+int main(int argc, char *argv[]) {
+  char path[4096];
+  snprintf(path, sizeof(path), "%s-so.so", argv[0]);
+  int i;
+
+  void *handle = dlopen(path, RTLD_LAZY);
+  if (!handle) fprintf(stderr, "%s\n", dlerror());
+  assert(handle != 0);
+  GetTls = (get_t)dlsym(handle, "GetTls");
+  assert(dlerror() == 0);
+
+  {
+    printf("Testing RTLD_DL_LINKMAP\n");
+    fflush(stdout);
+
+    Dl_info info;
+    struct link_map *map_ptr;
+    int ret = dladdr1(GetTls, &info, (void**)(&map_ptr), RTLD_DL_LINKMAP);
+    assert(ret != 0);
+    printf("fname: %s\n", info.dli_fname);
+    printf("fbase: %p\n", info.dli_fbase);
+    printf("sname: %s\n", info.dli_sname);
+    // CHECK: sname: GetTls
+    printf("saddr: %p\n", info.dli_saddr);
+
+    assert(map_ptr != NULL);
+    printf("map_ptr: %p\n", map_ptr);
+    fflush(stdout);
+
+    // Find the start of the link map
+    while(map_ptr->l_prev != NULL) {
+      fflush(stdout);
+      map_ptr = map_ptr->l_prev;
+    }
+
+    fflush(stdout);
+    while(map_ptr != NULL) {
+      assert(map_ptr->l_name != NULL);
+      printf("0x%lx: '%s', %p\n", map_ptr->l_addr, map_ptr->l_name, map_ptr->l_ld);
+      fflush(stdout);
+      map_ptr = map_ptr->l_next;
+    }
+    // CHECK: libc.so
+    // CHECK: dladdr1_test
+  }
+
+  // Test RTLD_DL_SYMENT
+
+  {
+    printf("Testing RTLD_DL_SYMENT\n");
+    fflush(stdout);
+
+    Dl_info info;
+    ElfW(Sym) *sym;
+    int ret = dladdr1(GetTls, &info, (void**)(&sym), RTLD_DL_SYMENT);
+    assert(ret != 0);
+    printf("fname: %s\n", info.dli_fname);
+    printf("fbase: %p\n", info.dli_fbase);
+    printf("sname: %s\n", info.dli_sname);
+    // CHECK: sname: GetTls
+    printf("saddr: %p\n", info.dli_saddr);
+
+    printf("sym: %d %d %d %d %lu %lu\n",
+           sym->st_name, sym->st_info, sym->st_other,
+           sym->st_shndx, sym->st_value, sym->st_size);
+    // CHECK: sym:
+  }
+  return 0;
+}
+#else  // BUILD_SO
+long var;
+long *GetTls() {
+  return &var;
+}
+#endif