Skip to content

build: fix ubsan oss-fuzz build.#5875

Merged
htuch merged 2 commits intoenvoyproxy:masterfrom
htuch:fix-ubsan-fuzz
Feb 7, 2019
Merged

build: fix ubsan oss-fuzz build.#5875
htuch merged 2 commits intoenvoyproxy:masterfrom
htuch:fix-ubsan-fuzz

Conversation

@htuch
Copy link
Member

@htuch htuch commented Feb 7, 2019

We were still failing ClusterFuzz builds for ubsan, another external_cmake regression.

Risk level: Low
Testing: oss-fuzz undefined Docker build.

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

We were still failing ClusterFuzz builds for ubsan, another external_cmake regression.

Risk level: Low
Testing: oss-fuzz undefined Docker build.

Signed-off-by: Harvey Tuch <htuch@google.com>
@htuch htuch requested review from dnoe and jmarantz February 7, 2019 17:37
jmarantz
jmarantz previously approved these changes Feb 7, 2019
Signed-off-by: Harvey Tuch <htuch@google.com>
@htuch htuch merged commit 6061f6c into envoyproxy:master Feb 7, 2019
@htuch htuch deleted the fix-ubsan-fuzz branch February 7, 2019 22:01
# it in the same test.
if "-static-libstdc++" in sys.argv[1:] or "-stdlib=libc++" in sys.argv[1:]:
if ("-static-libstdc++" in sys.argv[1:] or "-stdlib=libc++" in sys.argv[1:] or
"-std=c++0x" in sys.argv[1:]):
Copy link
Contributor

Choose a reason for hiding this comment

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

I cannot figure out why, but this change breaks local builds with --config libc++ (seems to be working fine on the CI, though...).

Copy link
Member Author

Choose a reason for hiding this comment

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

Have you looked any further into this? How would you like to move proceed?

Copy link
Contributor

Choose a reason for hiding this comment

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

I did not, and I won't have time to dig further into it anytime soon, sorry!

It's also unclear to me as to why did we need this change to fix UBSan on oss-fuzz, since everything else seems to work fine without it, and without access to oss-fuzz, it's really hard to fix this without breaking UBSan again.

Perhaps @lizan can dig into it? He was the one that originally asked for libc++ support.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was due to the fact that we invoke envoy_cc_wrapper for both CC and CXX for the external deps under external_cmake. The external_cmake build for some deps (e.g. zlib, c-ares) tries to build test/build binaries etc. that don't statically link, and doesn't supply the above flags, but it does set -std=c++0x. The use of the wrong compiler causes confusion specifically because of missing ubsan symbols. At least, that's roughly what I remember.

You can definitely repeat this yourself if you'd like, to run the oss-fuzz build, follow the instructions at https://github.com/envoyproxy/envoy/tree/master/test/fuzz#running-fuzz-tests-locally and replace --sanitizer=address with --sanitizer=undefined.

Copy link
Contributor

Choose a reason for hiding this comment

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

Moved to #5966 to track this.

fredlas pushed a commit to fredlas/envoy that referenced this pull request Mar 5, 2019
We were still failing ClusterFuzz builds for ubsan, another external_cmake regression.

Risk level: Low
Testing: oss-fuzz undefined Docker build.

Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
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.

4 participants