Skip to content

build: propagate ASAN/TSAN flags down to cmake_external deps.#6061

Closed
htuch wants to merge 10 commits intoenvoyproxy:masterfrom
htuch:fix-bazelrc-san
Closed

build: propagate ASAN/TSAN flags down to cmake_external deps.#6061
htuch wants to merge 10 commits intoenvoyproxy:masterfrom
htuch:fix-bazelrc-san

Conversation

@htuch
Copy link
Copy Markdown
Member

@htuch htuch commented Feb 25, 2019

While we await a solution for
bazel-contrib/rules_foreign_cc#154 (comment), this PR provides a
temporary workaround to get full ASAN/TSAN propagation as needed.

Risk level: Low
Testing: --config=clang-{tsan,asan}. This was broken strangely this morning on my distribution,
after some clang-7 package upgrade this morning, due to the mismatched linker/compiler flags we
previously had been sending to cmake_external builds.

Signed-off-by: Harvey Tuch htuch@google.com

While we await a solution for
bazel-contrib/rules_foreign_cc#154 (comment), this PR provides a
temporary workaround to get full ASAN/TSAN propagation as needed.

Risk level: Low
Testing: --config=clang-{tsan,asan}. This was broken strangely this morning on my distribution,
after some clang-7 package upgrade this morning, due to the mismatched linker/compiler flags we
previously had been sending to cmake_external builds.

Signed-off-by: Harvey Tuch <htuch@google.com>
@htuch
Copy link
Copy Markdown
Member Author

htuch commented Feb 25, 2019

CC @mergeconflict @irengrig

@lizan
Copy link
Copy Markdown
Member

lizan commented Feb 25, 2019

@htuch seems CI is failing, maybe you need newer image?

@htuch
Copy link
Copy Markdown
Member Author

htuch commented Feb 25, 2019

@lizan seems some mystery compile error in gperftools, no idea why it's suddenly missing munmap, will dig.

@htuch
Copy link
Copy Markdown
Member Author

htuch commented Feb 25, 2019

Hmm, checking for working mmap... no from the configure log, that's probably not good.

@PiotrSikora
Copy link
Copy Markdown
Contributor

Hmm, checking for working mmap... no from the configure log, that's probably not good.

You might need to update autoconf:
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=866406

@htuch
Copy link
Copy Markdown
Member Author

htuch commented Feb 26, 2019

@PiotrSikora thanks for that pointer, I think we have the same issue.

From config.log:

configure:18376: clang -o conftest -fsanitize=address,undefined -fno-sanitize=vptr -ggdb3 -fno-omit-frame-pointer -O2 -DNDEBUG  -lpthread conftest.c  >&5
configure:18376: $? = 0
configure:18376: ./conftest

=================================================================
==2690==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 4096 byte(s) in 1 object(s) allocated from:
    #0 0x4c2b43 in __interceptor_malloc (/build/tmp/_bazel_bazel/b570b5ccd0454dc9af9f65ab1833764d/external/envoy_deps_cache_b22e04bff96538ea37e715942da6315c/gperftools.dep.build/gperftools-fc00474ddc21fff618fc3f009b46590e241e425e/conftest+0x4c2b43)
    #1 0x4f40a8 in main /build/tmp/_bazel_bazel/b570b5ccd0454dc9af9f65ab1833764d/external/envoy_deps_cache_b22e04bff96538ea37e715942da6315c/gperftools.dep.build/gperftools-fc00474ddc21fff618fc3f009b46590e241e425e/conftest.c:217:20
    #2 0x7f8d3fa5882f in __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291

Direct leak of 4096 byte(s) in 1 object(s) allocated from:
    #0 0x4c2b43 in __interceptor_malloc (/build/tmp/_bazel_bazel/b570b5ccd0454dc9af9f65ab1833764d/external/envoy_deps_cache_b22e04bff96538ea37e715942da6315c/gperftools.dep.build/gperftools-fc00474ddc21fff618fc3f009b46590e241e425e/conftest+0x4c2b43)
    #1 0x4f3c72 in main /build/tmp/_bazel_bazel/b570b5ccd0454dc9af9f65ab1833764d/external/envoy_deps_cache_b22e04bff96538ea37e715942da6315c/gperftools.dep.build/gperftools-fc00474ddc21fff618fc3f009b46590e241e425e/conftest.c:168:19
    #2 0x7f8d3fa5882f in __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291

SUMMARY: AddressSanitizer: 8192 byte(s) leaked in 2 allocation(s).

.. and we seem to have an autoconf version prior to the bug fix ...

++ 1551140675.668187254 apt-cache show autoconf
Package: autoconf
Status: install ok installed
Priority: optional
Section: devel
Installed-Size: 1861
Maintainer: Ubuntu Developers <ubuntu-devel-discuss@lists.ubuntu.com>
Architecture: all
Multi-Arch: foreign
Version: 2.69-9
...

Will look into updating the base image.

Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Harvey Tuch <htuch@google.com>
ctxt.symlink(Label("//ci/prebuilt:BUILD"), "BUILD")

# Data files for build recipes
for f in ["gperftools.patch.gz"]:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be easier to update autoconf, or simply hardcode -DHAVE_MMAP to ignore the breaking test?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lizan expressed a preference for this approach; for release versions we have fixed autoconf outputs, we will revert back to this when we next move to a release version.

@htuch
Copy link
Copy Markdown
Member Author

htuch commented Feb 26, 2019

Looks like a legit TSAN failure, libevent does some technically unsafe things by writing to a shared boolean across threads (event_debug_mode_too_late). Filed libevent/libevent#777, will disable this debug support for TSAN builds.

Signed-off-by: Harvey Tuch <htuch@google.com>
@htuch
Copy link
Copy Markdown
Member Author

htuch commented Feb 26, 2019

I have a workaround for the libevent TSAN issues, next up is the ASAN issue in LuaJIT at https://circleci.com/gh/envoyproxy/envoy/172877?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link.

@htuch
Copy link
Copy Markdown
Member Author

htuch commented Feb 27, 2019

More TSAN errors, this time in dispatcher posting, will dig further tomorrow.

build:asan --copt -fsanitize=address,undefined
build:asan --linkopt -fsanitize=address,undefined
build:asan --copt -fno-sanitize=vptr
# TODO(htuch): these are temporary workarounds, remove when
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: It's not clear which are "these", put them in the bottom?

Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Harvey Tuch <htuch@google.com>
@htuch
Copy link
Copy Markdown
Member Author

htuch commented Mar 5, 2019

Uh oh, with the bump to Clang 7.0.1 in #5953 (and basically anyone else who wants to bump the CI image), we will hit failures like #5953.

@lizan is LuaJIT fixed with your LSAN_OPTIONS change? I think maybe we only have a some libevent TSAN issues re: atomics.

htuch added a commit to htuch/envoy that referenced this pull request Mar 6, 2019
While we await a solution for bazel-contrib/rules_foreign_cc#154 (comment), this PR provides a
temporary workaround to get full ASAN/TSAN propagation as needed.

This continues envoyproxy#6061, which had accumulated too much
complicated merge history to work with after the recent gperftools/LuaJIT migration to
rules_foreign_cc.

Risk level: Low
Testing: --config=clang-{tsan,asan}. This is broken in the latest CI images due to the mismatched
  linker/compiler flags we previously had been sending to cmake_external builds.

Signed-off-by: Harvey Tuch <htuch@google.com>
@htuch
Copy link
Copy Markdown
Member Author

htuch commented Mar 6, 2019

Closing out in favor of cleaned up #6196.

@htuch htuch closed this Mar 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants