[sanitizer] Intercept glibc's argp_parse()

[iii]

Summary

Glibc provides the argp_parse() function for parsing command line
arguments [1].

Indicate that argc/argv are read from and arg_index is written to.
Strictly speaking, we also need to indicate that argp is read from,
but this would require describing its layout, and most people use a
static initializer there, so it's not worth the effort.

[1] https://www.gnu.org/software/libc/manual/html_node/Argp.html

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

10385

I guess it should be always internal_strlen(argv[i]) + 1 ?
!strict_string_checks allows to pass non true C strings in API if we know that parser can return early.
Here I don't see why we should permit non zero terminate strings.

compiler-rt/test/sanitizer_common/TestCases/Linux/argp_parse.c

8

why only the trivial test case?

[iii]

• Drop strict_string_checks.
• Extend the testcase to check a more realistic use case.

[iii]

compiler-rt/test/sanitizer_common/TestCases/Linux/argp_parse.c

8

I've added a more realistic example here, please let me know if we need more, e.g., failures due to passing uninitialized or freed values in argv.

[vitalybuka]

compiler-rt/test/sanitizer_common/TestCases/Linux/argp_parse.c

44

what if you test on real argc and argv and pass them like "%run %t 1 2 3 4"
I suspect msan will complain because glibc does not mark them as initialized?
If so we need to remove COMMON_INTERCEPTOR_READ_RANGE.

[iii]

compiler-rt/test/sanitizer_common/TestCases/Linux/argp_parse.c

44

Good point, thanks! I've added the test and it works. I believe that argv is unpoisoned by ClearShadowForThreadStackAndTLS().

[iii]

• Added a test with real args/argv.

[iii]

Ping. Is there anything else that needs improvement?

[iii]

Ping.

[vitalybuka]

LGTM

[vitalybuka]

compiler-rt/test/sanitizer_common/TestCases/Linux/argp_parse.c

3

Please add

// This is GLIBC specific.
// UNSUPPORTED: android

3

Please add

// This is GLIBC specific.
// UNSUPPORTED: android

even
// UNSUPPORTED: android, target={{.*(freebsd|netbsd).*}}

[iii]

• Exclude Android and BSD.

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

INTERCEPTOR(void, hexdump, const void *ptr, int length, const char *header, int flags) {
   REAL(hexdump)(ptr, length, header, flags);
 }
 
 #define INIT_HEXDUMP COMMON_INTERCEPT_FUNCTION(hexdump);
 #else
 #define INIT_HEXDUMP
 #endif
 
+#if SANITIZER_INTERCEPT_ARGP_PARSE
+INTERCEPTOR(int, argp_parse, const struct argp *argp, int argc, char **argv,
+            unsigned flags, int *arg_index, void *input) {
+  void *ctx;
+  COMMON_INTERCEPTOR_ENTER(ctx, argp_parse, argp, argc, argv, flags, arg_index,
+                           input);
+  for (int i = 0; i < argc; i++)
+    COMMON_INTERCEPTOR_READ_RANGE(ctx, argv[i], internal_strlen(argv[i]) + 1);
+  int res = REAL(argp_parse)(argp, argc, argv, flags, arg_index, input);
+  if (!res && arg_index)
+    COMMON_INTERCEPTOR_WRITE_RANGE(ctx, arg_index, sizeof(int));
+  return res;
+}
+
+#define INIT_ARGP_PARSE COMMON_INTERCEPT_FUNCTION(argp_parse);
+#else
+#define INIT_ARGP_PARSE
+#endif
+
 #include "sanitizer_common_interceptors_netbsd_compat.inc"
 
 static void InitializeCommonInterceptors() {
 #if SI_POSIX
   static u64 metadata_mem[sizeof(MetadataHashMap) / sizeof(u64) + 1];
   interceptor_metadata_map = new ((void *)&metadata_mem) MetadataHashMap();
 #endif
 
#endif
   INIT_QSORT;
   INIT_QSORT_R;
   INIT_BSEARCH;
   INIT_SIGALTSTACK;
   INIT_PROCCTL
   INIT_UNAME;
   INIT___XUNAME;
   INIT_HEXDUMP;
+  INIT_ARGP_PARSE;
 
   INIT___PRINTF_CHK;
 }

compiler-rt/lib/sanitizer_common/sanitizer_platform_interceptors.h

 // calling it and assuming that it does not clobber registers.
 #define SANITIZER_INTERCEPT_SIGALTSTACK \
   (SI_POSIX && !(SANITIZER_APPLE && SANITIZER_I386))
 #define SANITIZER_INTERCEPT_UNAME (SI_POSIX && !SI_FREEBSD)
 #define SANITIZER_INTERCEPT___XUNAME SI_FREEBSD
 #define SANITIZER_INTERCEPT_FLOPEN SI_FREEBSD
 #define SANITIZER_INTERCEPT_PROCCTL SI_FREEBSD
 #define SANITIZER_INTERCEPT_HEXDUMP SI_FREEBSD
+#define SANITIZER_INTERCEPT_ARGP_PARSE SI_GLIBC
 
 // This macro gives a way for downstream users to override the above
 // interceptor macros irrespective of the platform they are on. They have
 // to do two things:
 // 1. Build compiler-rt with -DSANITIZER_OVERRIDE_INTERCEPTORS.
 // 2. Provide a header file named sanitizer_intercept_overriders.h in the
 //    include path for their compiler-rt build.
 // An example of an overrider for strlen interceptor that one can list in

compiler-rt/test/sanitizer_common/TestCases/Linux/argp_parse.c

+// RUN: %clang %s -o %t && %run %t -o baz
+
+// argp_parse is glibc specific.
+// UNSUPPORTED: android, target={{.*(freebsd|netbsd).*}}
+
+#include <argp.h>
+#include <assert.h>
+#include <stdlib.h>
+#include <string.h>
+
+struct test {
+  const char *option_value;
+};
+
+static const struct argp_option options[] = {
+    {"option", 'o', "OPTION", 0, "Option", 0},
+    {NULL, 0, NULL, 0, NULL, 0},
+};
+
+static error_t parser(int key, char *arg, struct argp_state *state) {
+  if (key == 'o') {
+    ((struct test *)(state->input))->option_value = arg;
+    return 0;
+  }
+  return ARGP_ERR_UNKNOWN;
+}
+
+static struct argp argp = {.options = options, .parser = parser};
+
+void test_nulls(char *argv0) {
+  char *argv[] = {argv0, NULL};
+  int res = argp_parse(NULL, 1, argv, 0, NULL, NULL);
+  assert(res == 0);
+}
+
+void test_synthetic(char *argv0) {
+  char *argv[] = {argv0, "-o", "foo", "bar", NULL};
+  struct test t = {NULL};
+  int arg_index;
+  int res = argp_parse(&argp, 4, argv, 0, &arg_index, &t);
+  assert(res == 0);
+  assert(arg_index == 3);
+  assert(strcmp(t.option_value, "foo") == 0);
+}
+
+void test_real(int argc, char **argv) {
+  struct test t = {NULL};
+  int arg_index;
+  int res = argp_parse(&argp, argc, argv, 0, &arg_index, &t);
+  assert(res == 0);
+  assert(arg_index == 3);
+  assert(strcmp(t.option_value, "baz") == 0);
+}
+
+int main(int argc, char **argv) {
+  test_nulls(argv[0]);
+  test_synthetic(argv[0]);
+  test_real(argc, argv);
+  return EXIT_SUCCESS;
+}