This is an archive of the discontinued LLVM Phabricator instance.

[lldb/Fuzzer] Add fuzzer for expression evaluator
ClosedPublic

Authored by cassanova on Jul 8 2022, 8:50 AM.

Details

Summary

This commit adds a fuzzer for LLDB's expression evaluator. The fuzzer takes a different approach than the current fuzzers present, and uses an approach that is currently being used for clang fuzzers.

Instead of fuzzing the evaluator with randomly mutated characters, protobufs are used to generate a subset of C++. This is then converted to valid C++ code and sent to the expression evaluator. In addition, libprotobuf_mutator is used to mutate the fuzzer's inputs from valid C++ code to valid C++ code, rather than mutating from valid code to total nonsense.

Diff Detail

Event Timeline

cassanova created this revision.Jul 8 2022, 8:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2022, 8:50 AM
Herald added a subscriber: mgorny. · View Herald Transcript
cassanova requested review of this revision.Jul 8 2022, 8:50 AM
cassanova updated this revision to Diff 443362.Jul 8 2022, 2:57 PM
cassanova edited the summary of this revision. (Show Details)

Building the expression evaluator fuzzer is now conditional on the CLANG_ENABLE_PROTO_FUZZER CMake variable being enabled.

Copying the source and header files from is no longer being done in the top-level CMake file, this is instead added to the subdirectories of the clang fuzzer.

The fuzzer uses Clang's CMake modules for libprotobuf_mutator instead of copying the module into LLDB.

Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2022, 2:57 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

The review shows the diff with the previous iteration of the patch. Can you generate a diff with the current top-of-tree?

cassanova updated this revision to Diff 443768.Jul 11 2022, 3:00 PM

Shows top-of-tree changes, however CMake generation fails that the ProtobufMutator target already exists for clang-fuzzer:

CMake Error at /opt/homebrew/Cellar/cmake/3.23.1_1/share/cmake/Modules/ExternalProject.cmake:3453 (add_custom_target):
  add_custom_target cannot create target "protobuf_mutator" because another
  target with the same name already exists.  The existing target is a custom
  target created in source directory
  "/Users/chelseacassanova/code/llvm-project/clang/tools/clang-fuzzer".
cassanova added inline comments.Jul 11 2022, 3:18 PM
lldb/tools/lldb-fuzzer/lldb-expression-fuzzer/CMakeLists.txt
21

Commenting out this line causes the project to generate, but I get file not found errors when including the protobuf errors when trying to build the fuzzer.

cassanova updated this revision to Diff 444074.Jul 12 2022, 2:03 PM

The ProtobufMutator CMake module will build targets for clang and lldb individually depending on which project is building the mutator, instead of both fuzzers trying to build the same target.

The expression fuzzer's source file only includes handle-cxx and proto-to-cxx directly, instead of including them from their folders.

The expression fuzzer's CMake file adds the clang-fuzzer binary directory as a include directory so that the lldb fuzzer does not need to generate a second copy of cxx_proto.pb.h and cxx_proto.pb.cc. It also requires the Protobuf library, grabs its definitions and includes the protobuf include dirs to prevent a protobuf header from not being found in the expression fuzzer source file.

mib added inline comments.Jul 12 2022, 2:14 PM
clang/cmake/modules/ProtobufMutator.cmake
4–5 ↗(On Diff #444074)

If feels wrong to me that the clang protobuf cmake module knows about lldb.

May be we should just have 2 separate files for clang and lldb

cassanova added inline comments.Jul 12 2022, 2:28 PM
clang/cmake/modules/ProtobufMutator.cmake
4–5 ↗(On Diff #444074)

My preferred solution to this was just creating a target called ${LLVM_VARIABLE_THAT_TELLS_YOU_THE_SUBPROJECT_NAME}_protobuf_mutator to avoid using if-statements and direct strings but it looks like clang and lldb aren't defined as subprojects unless they're being built standalone.

Also in the event that some other subproject wanted to use this library then this solution just gets messier. Having 2 different CMake module files for the clang and lldb sides each or putting protobuf mutator in a more central location is another way to approach this

JDevlieghere added inline comments.Jul 12 2022, 5:36 PM
clang/cmake/modules/ProtobufMutator.cmake
4–5 ↗(On Diff #444074)

I agree with Ismail. I think we can fix this issue by making it possible to set the PBM_PREFIX in the scope that includes ProtobufMutator.cmake like this:

In clang/cmake/modules/ProtobufMutator.cmake:

if (NOT PBM_PREFIX) 
  set (PBM_PREFIX protobuf_mutator)
endif()

In lldb/tools/lldb-fuzzer/lldb-expression-fuzzer/CMakeLists.txt:

set (PBM_PREFIX lldb_protobuf_mutator)
include(ProtobufMutator)

That said, I think an even better way to do this is by using a cached variable that keep track of whether we've already included the external project. That way we don't have to create two different targets that are essentially the same. If I'm following the code correctly, all the variables that we rely on can be computed without the ExternalProject_Add. I imagine something like:

if (NOT PBM_PROJECT_ADDED) 
  ExternalProject_Add(${PBM_PREFIX}
    PREFIX ${PBM_PREFIX}
    GIT_REPOSITORY https://github.com/google/libprotobuf-mutator.git
    GIT_TAG master
    CMAKE_ARGS -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}
    CMAKE_CACHE_ARGS -DCMAKE_C_COMPILER:FILEPATH=${CMAKE_C_COMPILER}
                     -DCMAKE_CXX_COMPILER:FILEPATH=${CMAKE_CXX_COMPILER}
    BUILD_BYPRODUCTS ${PBM_LIB_PATH} ${PBM_FUZZ_LIB_PATH}
    UPDATE_COMMAND ""
    INSTALL_COMMAND ""
    )
    set(PBM_PROJECT_ADDED TRUE CACHE BOOL
    "Whether the ProtoBufMutator external project was added")
endif()

I think that should preclude us from creating the target twice.

Updated the ProtobufMutator CMake module and expression fuzzer CMakeLists file so that the expression fuzzer will create its own target name in its CMake file, and the ProtobufMutator will not attempt to create another target if one already exists

Also updated the expression fuzzer's CMake file to create a directory to store expression fuzzer artifacts.

cassanova marked an inline comment as done.Jul 13 2022, 4:04 PM
JDevlieghere added inline comments.Jul 14 2022, 8:39 AM
lldb/tools/lldb-fuzzer/lldb-expression-fuzzer/cxx_proto.proto
1 ↗(On Diff #444361)

Do we still need a copy of this for LLDB?

cassanova marked an inline comment as done.Jul 14 2022, 12:17 PM
cassanova added inline comments.
lldb/tools/lldb-fuzzer/lldb-expression-fuzzer/cxx_proto.proto
1 ↗(On Diff #444361)

This file is used to generate the cxx_proto.pb.h which is included in the source file. Since we just use the clang-fuzzer directory as an include directory then we shouldn't need this file anymore

cassanova edited the summary of this revision. (Show Details)

Removed the cxx_proto.proto file since we include the headers that it generates from the clang side.

JDevlieghere accepted this revision.Jul 18 2022, 3:38 PM

A few nits about naming, but otherwise this LGTM.

lldb/tools/lldb-fuzzer/lldb-expression-fuzzer/lldb-expression-fuzzer.cpp
47 ↗(On Diff #444761)
51–52 ↗(On Diff #444761)
59 ↗(On Diff #444761)
62 ↗(On Diff #444761)
This revision is now accepted and ready to land.Jul 18 2022, 3:38 PM
This revision was automatically updated to reflect the committed changes.
cassanova marked 4 inline comments as done.Jul 22 2022, 2:33 PM

This broke building of Clang, when it's set up by symlinking llvm-project/clang into llvm-project/llvm/tools. In that case, cmake configure errors out like this:

-- Configuring done 
CMake Error in tools/clang/tools/clang-fuzzer/handle-cxx/CMakeLists.txt:
  Target "clangHandleCXX" INTERFACE_INCLUDE_DIRECTORIES property contains
  path:
 
    "/home/martin/code/llvm-project/llvm/tools/clang/tools/clang-fuzzer/handle-cxx/." 

  which is prefixed in the source directory.

See e.g. https://stackoverflow.com/questions/25676277/cmake-target-include-directories-prints-an-error-when-i-try-to-add-the-source for some discussion on the issue.

Can we revert this commit until this has been sorted out?

mib added a comment.Jul 22 2022, 3:26 PM

This broke building of Clang, when it's set up by symlinking llvm-project/clang into llvm-project/llvm/tools. In that case, cmake configure errors out like this:

-- Configuring done 
CMake Error in tools/clang/tools/clang-fuzzer/handle-cxx/CMakeLists.txt:
  Target "clangHandleCXX" INTERFACE_INCLUDE_DIRECTORIES property contains
  path:
 
    "/home/martin/code/llvm-project/llvm/tools/clang/tools/clang-fuzzer/handle-cxx/." 

  which is prefixed in the source directory.

See e.g. https://stackoverflow.com/questions/25676277/cmake-target-include-directories-prints-an-error-when-i-try-to-add-the-source for some discussion on the issue.

Can we revert this commit until this has been sorted out?

@mstorsjo I reverted Chelsea's patch in b797834748f1, since she's offline now. Do you have a link to a bot failure that would help us investigate the issue ?

Thanks!

This broke building of Clang, when it's set up by symlinking llvm-project/clang into llvm-project/llvm/tools. In that case, cmake configure errors out like this:

-- Configuring done 
CMake Error in tools/clang/tools/clang-fuzzer/handle-cxx/CMakeLists.txt:
  Target "clangHandleCXX" INTERFACE_INCLUDE_DIRECTORIES property contains
  path:
 
    "/home/martin/code/llvm-project/llvm/tools/clang/tools/clang-fuzzer/handle-cxx/." 

  which is prefixed in the source directory.

See e.g. https://stackoverflow.com/questions/25676277/cmake-target-include-directories-prints-an-error-when-i-try-to-add-the-source for some discussion on the issue.

Can we revert this commit until this has been sorted out?

@mstorsjo I reverted Chelsea's patch in b797834748f1, since she's offline now. Do you have a link to a bot failure that would help us investigate the issue ?

Thanks!

Thanks!

It’s not on a public buildbot but only in my local continuous builds unfortunately - but I’ll have look at the issue myself today - I was running out of time yesterday.

This broke building of Clang, when it's set up by symlinking llvm-project/clang into llvm-project/llvm/tools. In that case, cmake configure errors out like this:

-- Configuring done 
CMake Error in tools/clang/tools/clang-fuzzer/handle-cxx/CMakeLists.txt:
  Target "clangHandleCXX" INTERFACE_INCLUDE_DIRECTORIES property contains
  path:
 
    "/home/martin/code/llvm-project/llvm/tools/clang/tools/clang-fuzzer/handle-cxx/." 

  which is prefixed in the source directory.

See e.g. https://stackoverflow.com/questions/25676277/cmake-target-include-directories-prints-an-error-when-i-try-to-add-the-source for some discussion on the issue.

Can we revert this commit until this has been sorted out?

@mstorsjo I reverted Chelsea's patch in b797834748f1, since she's offline now. Do you have a link to a bot failure that would help us investigate the issue ?

So in short, I think this would work if you'd simply remove the newly added target_include_directories(clangHandleCXX PUBLIC .) in clang/tools/clang-fuzzer/handle-cxx/CMakeLists.txt and target_include_directories(clangProtoToCXX PUBLIC .) and target_include_directories(clangLoopProtoToCXX PUBLIC .) in clang/tools/clang-fuzzer/proto-to-cxx/CMakeLists.txt.

To reproduce the issue - instead of configuring llvm+clang+lldb by passing -DLLVM_ENABLE_PROJECTS="clang;lldb" make symlinks in llvm-project/llvm/tools pointing to llvm-project/clang and llvm-project/lldb. (I.e., cd llvm-project/llvm/tools; ln -s ../../clang .; ln -s ../../lldb .) This slightly affects the effective path at where CMake sees the clang files, which causes CMake to report the unexpected path overlap issue here.

I've tried to build all these fuzzers - I haven't really managed to build it all, but I think I'm fairly close. Anyway, I tried removing those target_include_directories() in that setup, and it didn't change anything, but maybe it only makes a difference if one gets further than what I got. (Changing PUBLIC into PRIVATE in those lines avoided the issue too.)