This is an archive of the discontinued LLVM Phabricator instance.

Integrate Kostya's clang-proto-fuzzer with LLVM.
ClosedPublic

Authored by morehouse on Aug 4 2017, 9:24 AM.

Details

Summary

The clang-proto-fuzzer models a subset of C++ as a protobuf and
uses libprotobuf-mutator to generate interesting mutations of C++
programs. Clang-proto-fuzzer has already found several bugs in
Clang (e.g., https://bugs.llvm.org/show_bug.cgi?id=33747,
https://bugs.llvm.org/show_bug.cgi?id=33749).

As with clang-fuzzer, clang-proto-fuzzer requires the following
cmake flags:

  • CMAKE_C_COMPILER=clang
  • CMAKE_CXX_COMPILER=clang++
  • LLVM_USE_SANITIZE_COVERAGE=YES // needed for libFuzzer
  • LLVM_USE_SANITIZER=Address // needed for libFuzzer

In addition, clang-proto-fuzzer requires:

  • CLANG_ENABLE_PROTO_FUZZER=ON

clang-proto-fuzzer also requires the following dependencies:

  • binutils // needed for libprotobuf-mutator
  • liblzma-dev // needed for libprotobuf-mutator
  • libz-dev // needed for libprotobuf-mutator
  • docbook2x // needed for libprotobuf-mutator
  • Recent version of protobuf [3.3.0 is known to work]

A working version of libprotobuf-mutator will automatically be
downloaded and built as an external project.

Implementation of clang-proto-fuzzer provided by Kostya
Serebryany.

https://bugs.llvm.org/show_bug.cgi?id=33829

Diff Detail

Repository
rL LLVM

Event Timeline

morehouse created this revision.Aug 4 2017, 9:24 AM
morehouse edited the summary of this revision. (Show Details)Aug 4 2017, 9:33 AM
thakis added a subscriber: thakis.Aug 4 2017, 11:22 AM

Why should this be part of llvm? This seems to come with very heavy dependencies (protobuf), and LLVM has historically tried to minimize the number of things it depends on.

kcc edited edge metadata.Aug 4 2017, 11:29 AM

Why should this be part of llvm? This seems to come with very heavy dependencies (protobuf), and LLVM has historically tried to minimize the number of things it depends on.

This fuzzer has already uncovered a few llvm bugs, so I hope it can be useful directly.
But more than that, I hope that a ready-to-use integration with structure-aware fuzzing will allow other researchers to experiment.
Having it as a side patch (bit-rotten in a few weeks after creation) will discourage most of the potential researchers from experiments.

I agree we don't want to bring heavy deps to LLVM, but this patch (AFAICT) doesn't bring any new deps to the default build. (at least this is the intent)

vitalybuka added inline comments.Aug 4 2017, 1:50 PM
clang/cmake/modules/ProtobufMutator.cmake
13 ↗(On Diff #109761)

Why this is debug?

clang/tools/clang-fuzzer/CMakeLists.txt
12 ↗(On Diff #109761)

You already download mutator, so maybe just DOWNLOAD_PROTOBUF and simplify this piece?

clang/tools/clang-fuzzer/ClangFuzzer.cpp
20 ↗(On Diff #109761)

Do we want replace this fuzzer? Why not just add another one?

clang/tools/clang-fuzzer/cxx_proto.proto
93 ↗(On Diff #109761)

message CxxInput {

required Function f = 1;
required int/enum opt_level = 2;
required enum tripple = 3;
required scalar-evolution-max-arith-depth ...

}

clang/tools/clang-fuzzer/proto-to-cxx/proto_to_cxx.cpp
22 ↗(On Diff #109761)

Not sure that macro here is justified
could you please replace with
std::ostream &operator<<(std::ostream &os, const BinaryOp& x) {

...

morehouse added inline comments.Aug 4 2017, 2:50 PM
clang/cmake/modules/ProtobufMutator.cmake
13 ↗(On Diff #109761)

I was just using what the libprotobuf-mutator readme suggested. But I can change it to use CMAKE_BUILD_TYPE instead.

clang/tools/clang-fuzzer/CMakeLists.txt
12 ↗(On Diff #109761)

That would be simpler if only protobuf-mutator needed protobuf. But since we need protobuf for some of the source files here, it would actually make this CMakeLists.txt more complicated since it would have to fish for the paths where protobuf mutator builds protobuf and then redefine variables.

clang/tools/clang-fuzzer/ClangFuzzer.cpp
20 ↗(On Diff #109761)

The idea was to keep ClangFuzzer and add ClangProtoFuzzer alongside it. However, the two share a fair amount of code, which was factored out into HandleCXX.

clang/tools/clang-fuzzer/cxx_proto.proto
93 ↗(On Diff #109761)

Interesting idea. This would allow for protobuf-mutator to choose different option combinations, if I understand correctly.

Is that worth adding to this initial patch, though?

vitalybuka added inline comments.Aug 4 2017, 3:02 PM
clang/tools/clang-fuzzer/CMakeLists.txt
12 ↗(On Diff #109761)

It's ok to share code, but I don't see fuzzer with accept string as is.

clang/tools/clang-fuzzer/cxx_proto.proto
16 ↗(On Diff #109761)

I'd suggest proto3

93 ↗(On Diff #109761)

yes, instead of CXX_FUZZ_MORE

morehouse added inline comments.Aug 4 2017, 3:35 PM
clang/tools/clang-fuzzer/ClangFuzzer.cpp
20 ↗(On Diff #109761)

It's ok to share code, but I don't see fuzzer with accept string as is.

That's exactly what this fuzzer is doing.

vitalybuka added inline comments.Aug 4 2017, 5:17 PM
clang/tools/clang-fuzzer/ClangFuzzer.cpp
20 ↗(On Diff #109761)

my mistake, I see now.

clang/tools/clang-fuzzer/cxx_proto.proto
16 ↗(On Diff #109761)

proto3 has no required, to avoid backward compatibility issues.
Same is useful for us, we don't wont to discard corpus if we drop some field in the future.

kcc added a reviewer: bogner.Aug 7 2017, 2:29 PM

+bogner@ FYI

clang/tools/clang-fuzzer/ExampleClangProtoFuzzer.cpp
25 ↗(On Diff #109761)

this is debug code, not worth having here, plz remove

34 ↗(On Diff #109761)

Remove "-mllvm", "-scalar-evolution-max-arith-depth=4".
It's there as a workaround for a performance bug (https://bugs.llvm.org/show_bug.cgi?id=33494) but it shouldn't be here.

35 ↗(On Diff #109761)

Remove this section.
In a later change, please allow to change the tripple (and any other flags) similar to https://reviews.llvm.org/D36275

clang/tools/clang-fuzzer/cxx_proto.proto
16 ↗(On Diff #109761)

I'm afraid it's much more convenient to have 'required' here.
How else could you express a binary op node?

93 ↗(On Diff #109761)

For now, keep it as is, please (see my other comment about flags)

morehouse updated this revision to Diff 110111.Aug 7 2017, 4:51 PM
  • Build protobuf-mutator with same build type as current build.
  • Remove unnecessary options from clang-proto-fuzzer.
  • Expand macro.
morehouse marked 6 inline comments as done.Aug 7 2017, 4:53 PM
kcc added a comment.Aug 7 2017, 5:21 PM

Why do we need LLVM_ENABLE_RTTI=ON here?

vitalybuka added inline comments.Aug 7 2017, 6:32 PM
clang/tools/clang-fuzzer/cxx_proto.proto
17 ↗(On Diff #110111)

//option cc_api_version = 2;

Please remove

clang/tools/clang-fuzzer/proto-to-cxx/CMakeLists.txt
7 ↗(On Diff #110111)

Formatting of this statement looks weird

clang/tools/clang-fuzzer/proto-to-cxx/proto_to_cxx.cpp
46 ↗(On Diff #110111)

OP looks to trivial to potentially get into conflicts with some 3rd party macro

50 ↗(On Diff #110111)
switch (x.op()) {
    BinaryOp::PLUS: os << "+"; break
    BinaryOp::MINUS: os << "-"; break
    BinaryOp::MUL: os << "*"; break

does not look bad to me

102 ↗(On Diff #110111)

please remove commented code

In D36324#834660, @kcc wrote:

Why do we need LLVM_ENABLE_RTTI=ON here?

Attempting to build without it yields all kinds of protobuf errors. For example:

morehouse updated this revision to Diff 110215.Aug 8 2017, 9:16 AM
  • Formatting and code cleanup.
morehouse marked 5 inline comments as done.Aug 8 2017, 9:17 AM
kcc added a comment.Aug 8 2017, 9:55 AM
In D36324#834660, @kcc wrote:

Why do we need LLVM_ENABLE_RTTI=ON here?

Attempting to build without it yields all kinds of protobuf errors. For example:

This is very strange, I'd like to understand more about this LLVM_ENABLE_RTTI.
ideally, we should avoid it.

morehouse updated this revision to Diff 110222.Aug 8 2017, 10:09 AM
  • Define GOOGLE_PROTOBUF_NO_RTTI to remove RTTI requirement.
morehouse edited the summary of this revision. (Show Details)Aug 8 2017, 10:10 AM
vitalybuka accepted this revision.Aug 8 2017, 11:03 AM
This revision is now accepted and ready to land.Aug 8 2017, 11:03 AM
kcc added a comment.Aug 8 2017, 11:20 AM

Looks good!
Now, please add a clang/tools/clang-fuzzer/README.txt describing how to build the fuzzers (both the old one and the new one) and how to run them.
For the new one explain how to install the deps

morehouse updated this revision to Diff 110262.Aug 8 2017, 12:57 PM
  • Add README.txt.
kcc accepted this revision.Aug 8 2017, 1:03 PM

LGTM with a couple if nits in the README

Thanks!

clang/tools/clang-fuzzer/README.txt
11 ↗(On Diff #110262)

.. of of Clang and LLVM

36 ↗(On Diff #110262)

(linux-only instructions)

51 ↗(On Diff #110262)

You may also build clang-fuzzer with this setup

morehouse updated this revision to Diff 110264.Aug 8 2017, 1:04 PM
  • Add run instructions to README.
morehouse updated this revision to Diff 110265.Aug 8 2017, 1:09 PM
  • README tweaks.
This revision was automatically updated to reflect the committed changes.
vitalybuka added inline comments.Aug 8 2017, 9:55 PM
clang/cmake/modules/ProtobufMutator.cmake
6 ↗(On Diff #109761)

Just noticed, for cmake projects shorter syntax can be used.
Example: https://github.com/google/libprotobuf-mutator/blob/master/cmake/external/googletest.cmake