[PATCH] D46545: [sanitizer] Move fgets, fputs and puts into sanitizer_common

[Peter Wu via Phabricator via llvm-commits]

Lekensteyn added a comment.

If a "good weather" test has to be added, I'll probably combine fgets/fputs/gets in a single sanitizer_common test.
In a future patch, I'll split the existing ASAN test into three separate programs.



================
Comment at: lib/esan/esan_interceptors.cpp:497
-  INTERCEPT_FUNCTION(fread);
-  INTERCEPT_FUNCTION(fwrite);
-  INTERCEPT_FUNCTION(puts);
----------------
FYI: this seemed a leftover after https://reviews.llvm.org/D31456 (https://github.com/google/sanitizers/issues/793)


================
Comment at: lib/msan/msan_interceptors.cc:752
-  ENSURE_MSAN_INITED();
-  InterceptorScope interceptor_scope;
-  char *res = REAL(fgets)(s, size, stream);
----------------
FYI: This was added in https://reviews.llvm.org/D42884, but I believe that the `COMMON_INTERCEPTOR_ENTER` macro in `lib/msan/msan_interceptors.cc` covers this now.


================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors.inc:1203
+  // to detect an overflow when the file contained '\0' bytes.
+  if (res) COMMON_INTERCEPTOR_WRITE_RANGE(ctx, s, size);
+  return res;
----------------
This is what I meant by "false negatives for msan". Possible choices for the length parameter:
- `REAL(strlen)(s)` / `internal_strlen` (as it did before in msan): if the read was short, then the buffer might not be fully initialized. MSAN could have caught this.
- `size` (proposed in this patch): even if the read was short, the full buffer is unpoisoned and MSAN would not complain about uninitialized memory.


================
Comment at: test/asan/TestCases/Posix/fgets_fputs.cc:2
+// RUN: %clangxx_asan -g %s -o %t
+// RUN: not %run %t 2>&1 | FileCheck %s --check-prefix=CHECK-FGETS
+// RUN: not %run %t 2 2>&1 | FileCheck %s --check-prefix=CHECK-FPUTS
----------------
vitalybuka wrote:
> could you please also create sanitizer_common test for each of moved functions?
Do you mean a test that checks that everything works fine with valid boundaries, for example `test/sanitizer_common/TestCases/Posix/access.cc`?
Otherwise, stack/heap OOB is not caught by msan, tsan, ubsan, only by ASAN. As for UAF, only ASAN and TSAN managed to detect that.

TSAN already has an additional test in test/tsan/race_on_puts.c. Should I create a similar race_on_fgets/race_on_fputs?

It seems that the previous fgets interceptor in the msan was not tested either, that could also be a point of improvement.


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D46545