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

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka requested changes to this revision.
vitalybuka added inline comments.
This revision now requires changes to proceed.


================
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;
----------------
Lekensteyn wrote:
> 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.
Let not mix refactoring with behavior change.
Please make this patch "move to sanitizer_common only" and a separate patch for "size vs strlen" (Also I am not yet convinced that we have to do so). 


================
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
----------------
Lekensteyn wrote:
> 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.
Yes, it's hard to test reports in sanitizer_common 
Just positive test which just make sure that interceptor does not crash.


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D46545