Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

split .bazelrc asan config into asan and ubsan #7947

Closed
wants to merge 1 commit into from

Conversation

yevgenypats
Copy link
Contributor

Description:
This splits the asan config in .bazelrc to ubsan and asan. This solves an issue with #7805 and this is a block for #7509. Because currently the asan-fuzzer doesnt work with ubsan.

Risk Level:
Low
Testing:
No

CC @htuch @asraa

@lizan
Copy link
Member

lizan commented Aug 16, 2019

@yevgenypats If you incorporate google/oss-fuzz#2664 to #7509 I guess it will unblock you there, can you try if that works?

@yevgenypats
Copy link
Contributor Author

@lizan I can try that. I already had a working fuzzer build that was based on oss-fuzz scripts that was working. @htuch wanted to have a native bazel build that will build the fuzzers so there won't be duplicate scripts building the same thing. I will happily do either but let's get to a consensus so we won't do duplicate work.

@lizan
Copy link
Member

lizan commented Aug 16, 2019

@yevgenypats Oh I missed that comment, re native bazel build, isn't https://github.com/envoyproxy/envoy/blob/master/.bazelrc#L144 already working? @asraa might know more.

@yevgenypats
Copy link
Contributor Author

Yes it almost works. it is blocked by this fix. @asraa was busy so I contributed. @asraa will be happy if you can look at the commit and see if this is ok.

@htuch
Copy link
Member

htuch commented Aug 19, 2019

I don't have any strong objections to this PR, I get what @yevgenypats is trying to do. My main concern is that we are doing this in terms of builds that are targeted at gcc, and clang is where we spend all our time these days on the fuzz and sanitizer front. @yevgenypats are you really planning on using gcc, or is this some kind of asan-base target that only exists for the Clang ASAN/UBSAN?

@htuch htuch self-assigned this Aug 19, 2019
@yevgenypats
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: asan (failed build)

🐱

Caused by: a #7947 (comment) was created by @yevgenypats.

see: more, trace.

@yevgenypats
Copy link
Contributor Author

@htuch I'm not planning on using gcc this is totally related to clang where I want to have an option not to use ubsan for some of the fuzzers because this makes life hard when trying to build it in unprivileged containers like in CircleCI.

Copy link
Member

@lizan lizan left a comment

Choose a reason for hiding this comment

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

I want to have an option not to use ubsan for some of the fuzzers because this makes life hard when trying to build it in unprivileged containers like in CircleCI.

Out of curiousity, which sanitizer need privileged containers?

The current build issue in this PR for ubsan is from vptr which I commented above, which I addressed in the oss-fuzz PR if you want to sanitize on vptr.

Anyway lets see whether #7949 works, which I think it does, then we don't need

build:asan --test_env=ASAN_SYMBOLIZER_PATH

# Basic UBSAN that works for gcc
build:ubsan --copt -fsanitize=undefined
build:ubsan --linkopt -fsanitize=undefined
Copy link
Member

Choose a reason for hiding this comment

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

You need move -fno-sanitize=vptr after this line.

@yevgenypats
Copy link
Contributor Author

Agree let's wait for #7949

@lizan lizan added the waiting label Aug 20, 2019
@lizan
Copy link
Member

lizan commented Aug 22, 2019

#7949 is landed, do you still need this one?

@yevgenypats
Copy link
Contributor Author

nope. we can remove this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants