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

[Envoy] Add project. #1130

Merged
merged 3 commits into from
Feb 2, 2018
Merged

[Envoy] Add project. #1130

merged 3 commits into from
Feb 2, 2018

Conversation

htuch
Copy link
Contributor

@htuch htuch commented Feb 1, 2018

@inferno-chromium
Copy link
Collaborator

@jonathanmetzman - can you please help to review.

Copy link
Contributor

@jonathanmetzman jonathanmetzman left a comment

Choose a reason for hiding this comment

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

I have a few nits about Dockerfile, but more importantly something weird is going on with coverage when using ASan.
If I build a coverage build of the fuzzer I see coverage reported as 10148 but when I do an ASan build I see it reported as 34.

I don't know what is causing this but I will need to look into it further. It's hard to tell how the fuzzer's performance is affected by this since the target appears pretty simple.

RUN apt-get update && apt-get install -y software-properties-common python-software-properties
RUN add-apt-repository ppa:webupd8team/java
RUN apt-get update && apt-get -y install \
vim \
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you actually need vim?

curl \
autoconf \
libtool
RUN apt-get -y install cmake wget golang
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: can you add these to the apt-get -y install command started on line 22 (like curl, autoconf , libtool etc.)?

MAINTAINER [email protected]

RUN apt-get update && apt-get install -y software-properties-common python-software-properties
RUN add-apt-repository ppa:webupd8team/java
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this line is needed. openjdk-8-jdk isn't downloaded from there.
I think line 20 is only needed for line 21 so please remove that as well if you remove this line.

@htuch
Copy link
Contributor Author

htuch commented Feb 1, 2018

Yep, the Docker stuff is largely because we started with a copy+paste of projects/grpc. Will fix.

On coverage, this is similar to my local experience, I was a bit confused about how I'm supposed to validate locally with pure ASAN and without LLVM coverage generation; see #1126 for the thread. Any assistance here appreciated, it's a bit mysterious to me still how we get coverage without instrumentation.

@kcc
Copy link
Contributor

kcc commented Feb 1, 2018

I've built the asan version and it looks fine.
The coverage is indeed tiny, but that's totally expected from a target that fuzzes base64.
I assume this is just a test target and you will have more. :)

Any assistance here appreciated,

I am bit lost. Which part doesn't work after the fix for #1126 ?

@kcc
Copy link
Contributor

kcc commented Feb 1, 2018

Interesting.
Running

./infra/helper.py build_fuzzers envoy 

works and produces good asan binary.

But then running

./infra/helper.py build_fuzzers --sanitizer coverage envoy

fails:

external/envoy_deps/thirdparty_build/lib/libcrypto.a(bcm.c.o):bcm.c:function RAND_bytes_with_additional_data: error: undefined reference to '__asan_stack_malloc_4'

could this be caused by #1125 (edited) ?

@kcc
Copy link
Contributor

kcc commented Feb 1, 2018

Yes, undoing #1125 helps.
I don't know enough about bazel, but maybe doing bazel clean at the top of build.sh would be sufficient

The coverage build also looks sane. Yes, it shows crazy INITED cov: 10124 (this is most likely caused different coverage instrumentations used. Should be safe to ignore).

@kcc
Copy link
Contributor

kcc commented Feb 1, 2018

Running ./infra/helper.py coverage envoy base64_fuzz_test
leads to

Navigate to see coverage: http://127.0.0.1:8001/

and a working top-level coverage page, but then

192.168.9.1 - - [01/Feb/2018 22:02:56] "GET /proc/self/cwd/source/common/common/base64.cc HTTP/1.1" 404 -

Looks like the local coverage collection is busted (apparently, noone have been using it for a while).

Anyway, this PR currently looks good to me (modulo comments about docker above).
Looking forward to seeing real fuzz targets (not just base64).

@kcc
Copy link
Contributor

kcc commented Feb 1, 2018

The problem with coverage may also be caused by the pesky "/proc/self/cwd/" which is created by bazel and thus be specific to this PR (and also affect grpc, but not others)

@htuch
Copy link
Contributor Author

htuch commented Feb 2, 2018

OK, great. Thanks for the review feedback @jonathanmetzman and @kcc. I've addressed the Dockerfile nits. Yes, we plan on doing more than base64 encoder fuzzing ;) The idea was to get a simple example working end-to-end and then building more complex control and data plane fuzzer binaries once the infrastructure was in place.

@inferno-chromium inferno-chromium requested a review from kcc February 2, 2018 15:15
@kcc kcc merged commit d0fc020 into google:master Feb 2, 2018
@inferno-chromium
Copy link
Collaborator

@kcc
Copy link
Contributor

kcc commented Feb 2, 2018

I suspect this is related to my comment #1130 (comment) above:
We either need to revert #1125 (helps) or add bazel clean to the build.sh (unverified guess)

htuch added a commit to htuch/oss-fuzz that referenced this pull request Feb 2, 2018
@htuch
Copy link
Contributor Author

htuch commented Feb 2, 2018

#1133; this was just a convenience for local use, let's drop if broken on build machines.

kcc pushed a commit that referenced this pull request Feb 2, 2018
* Revert "[Envoy] Add project. (#1130)"

This reverts commit d0fc020.

* Revert "Fix Skia compile (#1132)"

This reverts commit 4bf9e7f.

* Revert "Propose graphics magick for inclusion in OSS-Fuzz (#1131)"

This reverts commit cb277cc.

* Revert "[json-c] Add project (#1123)"

This reverts commit 31b0046.

* Revert "[infra] Update upload URL timeout to be the same as build timeout (#1112)"

This reverts commit 9215296.

* Revert "infra/helper: persist /root directory via bind mount. (#1125)"

This reverts commit b77745a.
@kcc
Copy link
Contributor

kcc commented Feb 3, 2018

ubsan build fails (--sanitizer undefined):

clang-6.0: error: invalid argument '-fsanitize=vptr' not allowed with '-fno-rtti'

This is a common problem. We probably need to fix it eventually in the clang driver.
for now please add -fno-sanitize=vptr as some other projects do.

inferno-chromium added a commit that referenced this pull request Feb 3, 2018
@inferno-chromium
Copy link
Collaborator

@htuch - build is still failing, please take a look on Monday. we need to get a first working build, otherwise it raises a ton of exceptions on our infrastructure.

@htuch htuch deleted the envoy branch February 4, 2018 04:36
@htuch
Copy link
Contributor Author

htuch commented Feb 4, 2018

@inferno-chromium is it failing on ASAN or UBSAN? I noticed that you've set the project.yaml to only ASAN now.

I built with python infra/helper.py build_fuzzers --sanitizer=undefined envoy after your recent changes to build.sh to fix CFLAGS/CXXFLAGS, it seems to work fine and I can run the fuzzer locally.

Beyond what I can see locally, are there any build debug artifacts you can point me at?

@inferno-chromium
Copy link
Collaborator

@htuch - i looked at wrong log incorrectly, this seemed to be failure during coverage build (see https://oss-fuzz-build-logs.storage.googleapis.com/index.html), so i have reenabled ubsan now. Also, right now i can build coverage locally fine, so triggered another envoy build on builder, lets see if it succeeds.

@htuch
Copy link
Contributor Author

htuch commented Feb 5, 2018

This looks like it's still failing. I think I understand the underlying issue. Our external dependencies are built outside of Bazel, and pick up on CFLAGS/CXXFLAGS. Our top-level build driver isn't smart enough right now to force a rebuild when they change. So, we do an ASAN build first, that's fine, then when we do UBSAN, we attempt to link a bunch of external deps built with the wrong flags. I will fix this in Envoy, it's a small change. Should be merged by later today or tomorrow. CC: @jmillikin-stripe

@inferno-chromium
Copy link
Collaborator

@htuch - should we do bazel clean in build.sh ?

@htuch
Copy link
Contributor Author

htuch commented Feb 5, 2018 via email

@inferno-chromium
Copy link
Collaborator

@htuch - ok upto you, we are fine to wait another day till this is fixed properly.

htuch added a commit to htuch/envoy that referenced this pull request Feb 5, 2018
This was a longstanding problem with external deps that manifested in
google/oss-fuzz#1130.

Signed-off-by: Harvey Tuch <[email protected]>
htuch added a commit to envoyproxy/envoy that referenced this pull request Feb 6, 2018
This was a longstanding problem with external deps that manifested in
google/oss-fuzz#1130.

Signed-off-by: Harvey Tuch <[email protected]>
@htuch
Copy link
Contributor Author

htuch commented Feb 6, 2018

@inferno-chromium can we kick off another run, now that envoyproxy/envoy#2534 has merged?

@inferno-chromium
Copy link
Collaborator

@htuch - done and got first green build, yippee, build page should update in < 30 min, but i checked the results on builder.

tmatth pushed a commit to tmatth/oss-fuzz that referenced this pull request Oct 22, 2018
* [Envoy] Add project.

Following the steps at
https://github.com/google/oss-fuzz/blob/master/docs/new_project_guide.md.

Signed-off-by: Harvey Tuch <[email protected]>

* Dockerfile review feedback.

Signed-off-by: Harvey Tuch <[email protected]>
tmatth pushed a commit to tmatth/oss-fuzz that referenced this pull request Oct 22, 2018
…e#1133)

* Revert "[Envoy] Add project. (google#1130)"

This reverts commit d0fc020.

* Revert "Fix Skia compile (google#1132)"

This reverts commit 4bf9e7f.

* Revert "Propose graphics magick for inclusion in OSS-Fuzz (google#1131)"

This reverts commit cb277cc.

* Revert "[json-c] Add project (google#1123)"

This reverts commit 31b0046.

* Revert "[infra] Update upload URL timeout to be the same as build timeout (google#1112)"

This reverts commit 9215296.

* Revert "infra/helper: persist /root directory via bind mount. (google#1125)"

This reverts commit b77745a.
tmatth pushed a commit to tmatth/oss-fuzz that referenced this pull request Oct 22, 2018
tmatth pushed a commit to tmatth/oss-fuzz that referenced this pull request Oct 22, 2018
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