[msan] Intercept *getxattr and *listxattr.

[eugenis]

Please don't add new interceptors to msan_interceptors.cc. That just creates future work to move them to sanitizer_common.

[kcc]

+100500

[earthdok]

Does this mean I have to test them under ASan as well?

[earthdok]

• move interceptors to common

[eugenis]

You need to update tsan_stat.h and tsan_stat.cc. Run check-tsan to verify.

lib/sanitizer_common/sanitizer_common_interceptors.inc

3074

if (path) COMMON_INTERCEPTOR_READ_RANGE(ctx, path, REAL(strlen)(path) + 1);

3076

if (res > 0 && list)

size => res

[glider]

Dima has removed tsan_stat.*, hasn't he?

[earthdok]

lib/sanitizer_common/sanitizer_common_interceptors.inc

3076

We should probably fix this also in sanitizer_common_syscalls.inc, then.

[earthdok]

lib/sanitizer_common/sanitizer_common_interceptors.inc

3076

Also, I'm fairly sure res > 0 implies list != NULL.

[earthdok]

Dima has removed tsan_stat.*, hasn't he?

The file is there but it no longer has interceptor stats.

[earthdok]

lib/sanitizer_common/sanitizer_common_interceptors.inc

3076

Ok, looking at the man page it actually says:

"An empty buffer of _size_ zero can be passed into these calls to return the current size of the list of extended attribute names"

So we should not attempt to unpoison if size == 0. (The way this is formulated still doesn't allow list == 0, but it's probably best to check.)

[earthdok]

• address comments by eugenis; fix sanitizer_common_syscalls.inc

[earthdok]

BTW, the code duplication between sanitizer_common_syscalls.inc and sanitizer_common_interceptors.inc is abhorrent. I think we should be able to reuse the PRE and POST hooks from sanitizer_common_syscalls.inc in sanitizer_common_interceptors.inc, since a lot of intercepted functions are simple syscall wrappers. Anyone disagree?

[eugenis]

It would be nice to reuse some code, but we must be careful. A lot of syscalls have subtle difference in semantics with libc wrappers, especially in return value.

Looks like your wrappers are still incorrect. As I understood from man pages, these functions return the desired buffer size, which may be larger than allocated size. The wording is not clear, please verify. In that case we need to write max(size, res), or something like that.

In general, we write interceptors defensively, verifying that arguments make sense even if the doc says they must. Man pages are not specs, they are often wrong; and not everyone codes to specs.

It might be a good idea to move checks inside INTERCEPTOR_WRITE_RANGE / INTERCEPTOR_READ_RANGE in another refactoring CL.

[earthdok]

In that case we need to write max(size, res), or something like that.

This would attempt to unpoison more memory than we actually have.

I think the wording is clear: if a buffer of zero size is passed, then the desired size is returned. Nothing is written to the buffer, it just tells us what kind of buffer is wanted. So no need to unpoison anything in this case.

[eugenis]

LGTM

lib/msan/lit_tests/Linux/xattr.cc

9

This will fail if libattr is not installed. I think we need to check it either in configure/cmake or in this test, and pass if not found.

lib/sanitizer_common/sanitizer_common_interceptors.inc

3076

Please add a comment here about size==0 being a special case.

[earthdok]

• do not include libxattr headers; add comment

[earthdok]

Committed r200464

[eugenis]

LGTM

lib/msan/lit_tests/Linux/xattr.cc

+// RUN: %clangxx_msan -m64 -O0 %s -o %t && %t %p 2>&1
+// RUN: %clangxx_msan -m64 -O0 -D_FILE_OFFSET_BITS=64 %s -o %t && %t %p 2>&1
+// RUN: %clangxx_msan -m64 -O3 %s -o %t && %t %p 2>&1
+
+#include <argz.h>
+#include <assert.h>
+#include <sys/types.h>
+#include <attr/xattr.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include <sanitizer/msan_interface.h>
+
+char g_path[1024];
+int g_fd;
+
+// Life before closures...
+ssize_t listxattr_wrapper(char *buf, size_t size) {
+  return listxattr(g_path, buf, size);
+}
+
+ssize_t llistxattr_wrapper(char *buf, size_t size) {
+  return llistxattr(g_path, buf, size);
+}
+
+ssize_t flistxattr_wrapper(char *buf, size_t size) {
+  return flistxattr(g_fd, buf, size);
+}
+
+ssize_t getxattr_wrapper(const char *name, char *buf, size_t size) {
+  return getxattr(g_path, name, buf, size);
+}
+
+ssize_t lgetxattr_wrapper(const char *name, char *buf, size_t size) {
+  return lgetxattr(g_path, name, buf, size);
+}
+
+ssize_t fgetxattr_wrapper(const char *name, char *buf, size_t size) {
+  return fgetxattr(g_fd, name, buf, size);
+}
+
+size_t test_list(ssize_t fun(char*, size_t), char **buf) {
+  int buf_size = 1024;
+  while (true) {
+    *buf = (char *)malloc(buf_size);
+    assert(__msan_test_shadow(*buf, buf_size) != -1);
+    ssize_t res = fun(*buf, buf_size);
+    if (res >= 0) {
+      assert(__msan_test_shadow(*buf, buf_size) == -1);
+      return res;
+    }
+    if (errno == ENOTSUP) {
+      printf("Extended attributes are disabled. *xattr test is a no-op.\n");
+      exit(0);
+    }
+    assert(errno == ERANGE);
+    free(*buf);
+    buf_size *= 2;
+  }
+}
+
+// True means success. False means result inconclusive because we don't have
+// access to this attribute.
+bool test_get_single_attr(ssize_t fun(const char *, char *, size_t),
+                          const char *attr_name) {
+  char *buf;
+  int buf_size = 1024;
+  while (true) {
+    buf = (char *)malloc(buf_size);
+    assert(__msan_test_shadow(buf, buf_size) != -1);
+    ssize_t res = fun(attr_name, buf, buf_size);
+    if (res >= 0) {
+      assert(__msan_test_shadow(buf, buf_size) == -1);
+      free(buf);
+      return true;
+    }
+    if (errno == ENOTSUP) {
+      printf("Extended attributes are disabled. *xattr test is a no-op.\n");
+      exit(0);
+    }
+    if (errno == ENOATTR)
+      return false;
+    assert(errno == ERANGE);
+    free(buf);
+    buf_size *= 2;
+  }
+}
+
+void test_get(ssize_t fun(const char *, char *, size_t), const char *attr_list,
+              size_t attr_list_size) {
+  // Try every attribute, until we see one we can access. Attribute names are
+  // null-separated strings in attr_list.
+  size_t attr_list_len = argz_count(attr_list, attr_list_size);
+  char **attrs = (char **)malloc((attr_list_len + 1) * sizeof(char *));
+  size_t i;
+  for (i = 0; (i < attr_list_len) && attrs[i]; i++) {
+    if (test_get_single_attr(fun, attrs[i]))
+      return;
+  }
+  printf("*xattr test could not access any attributes.\n");
+}
+
+// TODO: set some attributes before trying to retrieve them with *getxattr.
+// Currently the list is empty, so *getxattr is not tested.
+int main(int argc, char *argv[]) {
+  assert(argc == 2);
+  snprintf(g_path, sizeof(g_path), "%s/%s", argv[1], "xattr_test_root/a");
+
+  g_fd = open(g_path, O_RDONLY);
+  assert(g_fd);
+
+  char *attr_list;
+  size_t attr_list_size;
+  attr_list_size = test_list(listxattr_wrapper, &attr_list);
+  free(attr_list);
+  attr_list_size = test_list(llistxattr_wrapper, &attr_list);
+  free(attr_list);
+  attr_list_size = test_list(flistxattr_wrapper, &attr_list);
+
+  test_get(getxattr_wrapper, attr_list, attr_list_size);
+  test_get(lgetxattr_wrapper, attr_list, attr_list_size);
+  test_get(fgetxattr_wrapper, attr_list, attr_list_size);
+
+  free(attr_list);
+  return 0;
+}

lib/sanitizer_common/sanitizer_common_interceptors.inc

if (res != (__sanitizer_clock_t)-1 && tms)
     COMMON_INTERCEPTOR_WRITE_RANGE(ctx, tms, struct_tms_sz);
   return res;
 }
 #define INIT_TIMES COMMON_INTERCEPT_FUNCTION(times);
 #else
 #define INIT_TIMES
 #endif
 
+#if SANITIZER_INTERCEPT_LISTXATTR
+INTERCEPTOR(SSIZE_T, listxattr, const char *path, char *list, SIZE_T size) {
+  void *ctx;
+  COMMON_INTERCEPTOR_ENTER(ctx, listxattr, path, list, size);
+  COMMON_INTERCEPTOR_READ_RANGE(ctx, path, REAL(strlen)(path));
+  SSIZE_T res = REAL(listxattr)(path, list, size);
+  if (res >= 0) COMMON_INTERCEPTOR_WRITE_RANGE(ctx, list, size);
+  return res;
+}
+INTERCEPTOR(SSIZE_T, llistxattr, const char *path, char *list, SIZE_T size) {
+  void *ctx;
+  COMMON_INTERCEPTOR_ENTER(ctx, llistxattr, path, list, size);
+  COMMON_INTERCEPTOR_READ_RANGE(ctx, path, REAL(strlen)(path));
+  SSIZE_T res = REAL(llistxattr)(path, list, size);
+  if (res >= 0) COMMON_INTERCEPTOR_WRITE_RANGE(ctx, list, size);
+  return res;
+}
+INTERCEPTOR(SSIZE_T, flistxattr, int fd, char *list, SIZE_T size) {
+  void *ctx;
+  COMMON_INTERCEPTOR_ENTER(ctx, flistxattr, fd, list, size);
+  SSIZE_T res = REAL(flistxattr)(fd, list, size);
+  if (res >= 0) COMMON_INTERCEPTOR_WRITE_RANGE(ctx, list, size);
+  return res;
+}
+#define INIT_LISTXATTR                   \
+  COMMON_INTERCEPT_FUNCTION(listxattr);  \
+  COMMON_INTERCEPT_FUNCTION(llistxattr); \
+  COMMON_INTERCEPT_FUNCTION(flistxattr);
+#else
+#define INIT_LISTXATTR
+#endif
+
+#if SANITIZER_INTERCEPT_GETXATTR
+INTERCEPTOR(SSIZE_T, getxattr, const char *path, const char *name, char *value,
+            SIZE_T size) {
+  void *ctx;
+  COMMON_INTERCEPTOR_ENTER(ctx, getxattr, path, name, value, size);
+  COMMON_INTERCEPTOR_READ_RANGE(ctx, path, REAL(strlen)(path));
+  COMMON_INTERCEPTOR_READ_RANGE(ctx, name, REAL(strlen)(name));
+  SSIZE_T res = REAL(getxattr)(path, name, value, size);
+  if (res >= 0) COMMON_INTERCEPTOR_WRITE_RANGE(ctx, value, size);
+  return res;
+}
+INTERCEPTOR(SSIZE_T, lgetxattr, const char *path, const char *name, char *value,
+            SIZE_T size) {
+  void *ctx;
+  COMMON_INTERCEPTOR_ENTER(ctx, lgetxattr, path, name, value, size);
+  COMMON_INTERCEPTOR_READ_RANGE(ctx, path, REAL(strlen)(path));
+  COMMON_INTERCEPTOR_READ_RANGE(ctx, name, REAL(strlen)(name));
+  SSIZE_T res = REAL(lgetxattr)(path, name, value, size);
+  if (res >= 0) COMMON_INTERCEPTOR_WRITE_RANGE(ctx, value, size);
+  return res;
+}
+INTERCEPTOR(SSIZE_T, fgetxattr, int fd, const char *name, char *value,
+            SIZE_T size) {
+  void *ctx;
+  COMMON_INTERCEPTOR_ENTER(ctx, fgetxattr, fd, name, value, size);
+  COMMON_INTERCEPTOR_READ_RANGE(ctx, name, REAL(strlen)(name));
+  SSIZE_T res = REAL(fgetxattr)(fd, name, value, size);
+  if (res >= 0) COMMON_INTERCEPTOR_WRITE_RANGE(ctx, value, size);
+  return res;
+}
+#define INIT_GETXATTR                   \
+  COMMON_INTERCEPT_FUNCTION(getxattr);  \
+  COMMON_INTERCEPT_FUNCTION(lgetxattr); \
+  COMMON_INTERCEPT_FUNCTION(fgetxattr);
+#else
+#define INIT_GETXATTR
+#endif
+
+
 #define SANITIZER_COMMON_INTERCEPTORS_INIT \
   INIT_TEXTDOMAIN;                         \
   INIT_STRCMP;                             \
   INIT_STRNCMP;                            \
   INIT_STRCASECMP;                         \
   INIT_STRNCASECMP;                        \
   INIT_READ;                               \
   INIT_PREAD;                              \
#define SANITIZER_COMMON_INTERCEPTORS_INIT \
   INIT_SINCOS;                             \
   INIT_REMQUO;                             \
   INIT_LGAMMA;                             \
   INIT_LGAMMA_R;                           \
   INIT_DRAND48_R;                          \
   INIT_GETLINE;                            \
   INIT_ICONV;                              \
   INIT_TIMES;                              \
+  INIT_LISTXATTR;                          \
+  INIT_GETXATTR;
 /**/

lib/sanitizer_common/sanitizer_platform_interceptors.h

 #define SANITIZER_INTERCEPT_GETLINE SI_LINUX_NOT_ANDROID
 
 #define SANITIZER_INTERCEPT__EXIT SI_LINUX
 
 #define SANITIZER_INTERCEPT_PHTREAD_MUTEX SI_NOT_WINDOWS
 #define SANITIZER_INTERCEPT_PTHREAD_COND SI_NOT_WINDOWS
 #define SANITIZER_INTERCEPT_PTHREAD_SETNAME_NP SI_LINUX_NOT_ANDROID
 
+#define SANITIZER_INTERCEPT_LISTXATTR SI_LINUX
+#define SANITIZER_INTERCEPT_GETXATTR SI_LINUX
+
 #endif  // #ifndef SANITIZER_PLATFORM_INTERCEPTORS_H