Regression(d437fba8ef626b6d8b7928540f630163a9b04021): msan doesn't correctly instrument memcpy() with -D_FORTIFY_SOURCE=2

[nico]

Bugzilla Link	44779
Version	unspecified
OS	Linux
Attachments	repro
CC	@eugenis,@rnk,@serge-sans-paille

Extended Description

Not sure if this is a clang bug or a runtime bug.

Link to discussion of bad commit: https://reviews.llvm.org/D71082

Unzip the attached repro.cc and run:

third_party/llvm-build/Release+Asserts/bin/clang++ -D_FORTIFY_SOURCE=2 -O2 -gline-tables-only -fsanitize=memory -fsanitize-memory-track-origins=2 -std=c++14 -fno-exceptions -fno-rtti ./base/repro.cc -o repro

./repro

Before d437fba that worked fine. After, it yields:

• ./repro
  ==93469==WARNING: MemorySanitizer: use-of-uninitialized-value
  #​0 0x4d199a in (anonymous namespace)::itanium_demangle::PODSmallVector<(anonymous namespace)::itanium_demangle::Node*, 8ul>::push_back((anonymous namespace)::itanium_demangle::Node* const&) /usr/local/google/home/thakis/src/chrome/src/./base/repro.cc:2588:9
  #​1 0x4ac008 in (anonymous namespace)::itanium_demangle::AbstractManglingParser<(anonymous namespace)::itanium_demangle::ManglingParser, Allocator>::parseTemplateArgs(bool) /usr/local/google/home/thakis/src/chrome/src/./base/repro.cc:5808:30
  #​2 0x4a4a3b in parseNestedName /usr/local/google/home/thakis/src/chrome/src/./base/repro.cc:3471:31
  #​3 0x4a4a3b in (anonymous namespace)::itanium_demangle::AbstractManglingParser<(anonymous namespace)::itanium_demangle::ManglingParser, Allocator>::parseName((anonymous namespace)::itanium_demangle::AbstractManglingParser<(anonymous namespace)::itanium_demangle::ManglingParser, Allocator>::NameState*) /usr/local/google/home/thakis/src/chrome/src/./base/repro.cc:2845:25
  #​4 0x49ba91 in (anonymous namespace)::itanium_demangle::AbstractManglingParser<(anonymous namespace)::itanium_demangle::ManglingParser, Allocator>::parseEncoding() /usr/local/google/home/thakis/src/chrome/src/./base/repro.cc:5420:29
  #​5 0x49ad0b in parse /usr/local/google/home/thakis/src/chrome/src/./base/repro.cc:5827:35
  #​6 0x49ad0b in main /usr/local/google/home/thakis/src/chrome/src/./base/repro.cc:5924:40
  #​7 0x7fc8bb947bba in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x26bba)
  #​8 0x41f299 in _start (/usr/local/google/home/thakis/src/chrome/src/repro+0x41f299)

  Uninitialized value was stored to memory at
  #​0 0x445316 in __msan_memcpy /b/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/msan/msan_interceptors.cpp:1567:3
  #​1 0x4ac09e in memcpy /usr/include/x86_64-linux-gnu/bits/string_fortified.h:34:10
  #​2 0x4ac09e in operator= /usr/local/google/home/thakis/src/chrome/src/./base/repro.cc:2566:7
  #​3 0x4ac09e in (anonymous namespace)::itanium_demangle::AbstractManglingParser<(anonymous namespace)::itanium_demangle::ManglingParser, Allocator>::parseTemplateArgs(bool) /usr/local/google/home/thakis/src/chrome/src/./base/repro.cc:5797:22

  Uninitialized value was created by an allocation of 'OldParams' in the stack frame of function '_ZN12_GLOBAL__N_116itanium_demangle22AbstractManglingParserINS0_14ManglingParserI9AllocatorEES3_E17parseTemplateArgsEb'
  #​0 0x4ab420 in (anonymous namespace)::itanium_demangle::AbstractManglingParser<(anonymous namespace)::itanium_demangle::ManglingParser, Allocator>::parseTemplateArgs(bool) /usr/local/google/home/thakis/src/chrome/src/./base/repro.cc:5780

SUMMARY: MemorySanitizer: use-of-uninitialized-value /usr/local/google/home/thakis/src/chrome/src/./base/repro.cc:2588:9 in (anonymous namespace)::itanium_demangle::PODSmallVector<(anonymous namespace)::itanium_demangle::Node*, 8ul>::push_back((anonymous namespace)::itanium_demangle::Node* const&)
Exiting

[nico]

Copying over https://reviews.llvm.org/D71082#1857702 :

MemorySanitizer requires all code, including system libraries, to be instrumented. Else it reports false positives. For glibc, it has lots of interceptors to make sure glibc doesn't have to be instrumented (as far as I understand). So people usually don't have an instrumented glibc in their sysroot. But after this change, memcpy calls aren't always intercepted.

[serge-sans-paille]

Does that mean we should provide the correct interceptors for all the _builtin*_chk function? The actual effect of the original patch is to fix a bad behavior of clang, that totally ignored the glibc overrides for all the fortified source. It would be a regression to ignore them again.

It's time to go to bed in my TZ, but I'll have a look to your reproducer tomorrow.

[eugenis]

Yes, adding interceptors for _chk symbols would be the proper fix.
It's a non-trivial amount of work though. Consider suppressing this change when compiling with sanitizers as a short term fix, as George suggested in the code review.

[rnk]

It looks like this has come up for various sanitizers in the past:
google/sanitizers#689
google/sanitizers#247

Skimming those issues suggests that the standard guidance is "don't do that", disable _FORTIFY_SOURCE with sanitizers for the best experience.

So, I weakly support not reverting, and marking wontfix.

The most reasonable thing is probably to warn somewhere in the driver if we see -D_FORTIFY_SOURCE + MSan.

[nico]

All suggestions here sound fine to me. One comment:

The most reasonable thing is probably to warn somewhere in the driver if we see -D_FORTIFY_SOURCE + MSan.

MSan shouldn't be special. MSan requires complete instrumentation, but it helps tsan and the other sanitizers too. So if we go this route, the warning should be on all sanitizers except ubsan / cfi (similar to what gbiv said on the review).

[nico]

sguelton: ping?

[serge-sans-paille]

@​NicoWeber : I haven't worked on that bug yet.