apply patches needed for fuzz introspector integration by Navidem · Pull Request #7122 · google/oss-fuzz

Navidem · 2022-01-11T21:47:12Z

Imitating the changes from introspector diff along with the needed modifications.

DavidKorczynski

Two quick comments

infra/base-images/base-builder/Dockerfile

oliverchang

nice, this is a great start!

I left some suggestions to remove duplication and make things a bit cleaner.

oliverchang · 2022-01-12T02:48:49Z

infra/base-images/base-builder/compile

+  mv /usr/bin/ar /usr/bin/old-ar
+  mv /usr/bin/ranlib /usr/bin/old-ranlib
+
+  ln -s /usr/local/bin/llvm-ar /usr/bin/ar


Let's be more explicit here and store the patched introspector binaries in /usr/local/bin/introspector/llvm-ar etc.

I misunderstood what this was doing. This is just replacing the system versions of ar/ranlib with the built llvm versions (which happens to be the introspector patched one in this case).

Hmmm...Also, is this script idempotent. Might be a good idea to make it so to help with debugging.

oliverchang · 2022-01-12T02:49:40Z

infra/helper.py

      'FUZZING_ENGINE=' + engine,
      'SANITIZER=' + sanitizer,
      'ARCHITECTURE=' + architecture,
+      'GITHUB_REPO=',  #TODO: fix this to be loaded through main_repo property in project.yaml


nit: space after '#', and use 'TODO(Navidem)'.

i.e.

# TODO(Navidem): Fix this to be ...

oliverchang · 2022-01-12T03:00:33Z

infra/base-images/base-clang-introspector/checkout_build_install_llvm.sh

+echo "Using LLVM revision: $LLVM_REVISION"
+
+### For fuzz introspector
+echo "Applying introspector changes"


To avoid lots of code duplication, we can edit this file in base-clang instead.

We can just guard this with:

if [ -n "$INTROSPECTOR_PATCHES ]; then ... fi

Then we can get rid of the base-clang-introspector dir completely ,and modify base-clangs Dockerfile to do something like this using a runtime build arg.

ARG introspector ENV INTROSPECTOR_PATCHES=$instrospector

So that we can use the same Dockerfile to build both vanilla clang and patched clang:

docker build -t gcr.io/oss-fuzz-base/base-clang infra/base-images/base-clang # vanilla docker build --build-arg introspector=1 -t gcr.io/oss-fuzz-base/base-clang:introspector infra/base-images/base-clang # introspector

oliverchang · 2022-01-12T03:01:13Z

infra/base-images/base-runner/coverage

      $shared_libraries $LLVM_COV_COMMON_ARGS > $FUZZER_STATS_DIR/$target.json

+  # For introspector
+  llvm-cov show -instr-profile=$profdata_file -object=$target -line-coverage-gt=0 $shared_libraries $LLVM_COV_COMMON_ARGS > ${FUZZER_STATS_DIR}/$target.covreport


Does this add a significant amount of extra processing time?

not significantly in the few cases that I tried.

DavidKorczynski · 2022-01-12T13:53:03Z

Is it worth considering whether we should simply include this in base-clang rather than creating a whole new image base-clang-introspector? Point being the only patching we do of clang is insert a call to our pass in the LTO pipeline, which is a very non-intrusive patching and I would not expect breakages to happen. Notice there were no fuzz-introspector changes required going from LLVM 12 to LLVM 14 - the delay happened because I did not know about the -flegacy-pass-manager flag.

If using base-clang we can use environment variables to control whether the fuzz-introspector pass should run or not, and also which ar to use etc. I think this might be conceptually quite similar to how afl++ is used. This might be easier to maintain.

EDIT: these questions have been discussed here and the conclusion is keeping base-clang-introspector

jonathanmetzman · 2022-01-12T13:48:28Z

infra/base-images/base-builder/compile

+  mv /usr/bin/ar /usr/bin/old-ar
+  mv /usr/bin/ranlib /usr/bin/old-ranlib
+
+  ln -s /usr/local/bin/llvm-ar /usr/bin/ar


jonathanmetzman · 2022-01-12T13:49:22Z

infra/base-images/base-builder/compile

 fi

+if [ "$SANITIZER" = "introspector" ]; then
+  echo "We are in the introspector instrumentor"


I think we echo every command, so this isn't necessary to know we are using introspector.

jonathanmetzman · 2022-01-12T14:07:38Z

infra/base-images/base-builder/compile

+if [ "$SANITIZER" = "introspector" ]; then
+  unset CXXFLAGS
+  unset CFLAGS
+  apt-get install -y libjpeg-dev zlib1g-dev


FYI installing packages like this can mess up compilation of some projects I think

infra/base-images/base-builder/compile

jonathanmetzman · 2022-01-12T14:08:36Z

infra/base-images/base-builder/compile

+  apt-get install -y libjpeg-dev zlib1g-dev
+  pip3 install --upgrade setuptools
+  pip3 install cxxfilt pyyaml beautifulsoup4 lxml soupsieve matplotlib
+  mkdir $SRC/inspector-tmp


nit: Just call it inspector, not inspector-tmp

jonathanmetzman · 2022-01-12T14:10:17Z

infra/base-images/base-runner/coverage

  llvm-cov export -summary-only -instr-profile=$profdata_file -object=$target \
      $shared_libraries $LLVM_COV_COMMON_ARGS > $FUZZER_STATS_DIR/$target.json

+  # For introspector


nit: end this with a period.

jonathanmetzman · 2022-01-12T14:12:23Z

infra/base-images/base-builder/compile

+
+  find $SRC/ -name "*.data" -exec cp {} $SRC/inspector-tmp/ \;
+  find $SRC/ -name "*.data.yaml" -exec cp {} $SRC/inspector-tmp/ \;
+  # Move coverage report


end with peirod.

jonathanmetzman · 2022-01-12T14:13:57Z

infra/base-images/base-clang-introspector/Dockerfile

@@ -0,0 +1,51 @@
+# Copyright 2016 Google Inc.


got rid of the dir.

oliverchang

nice!!!

oliverchang · 2022-01-13T02:13:32Z

Please fix the presubmit issue here: https://github.com/google/oss-fuzz/runs/4797854485?check_suite_focus=true

oliverchang

LGTM with one last question!

oliverchang · 2022-01-14T01:09:11Z

infra/base-images/base-clang/checkout_build_install_llvm.sh

 apt-get update && apt-get install -y $LLVM_DEP_PACKAGES --no-install-recommends

+if [ -n "$INTROSPECTOR_PATCHES" ]; then
+  apt-get install -y texinfo bison flex


Can we delete these after the install?

will look into it.

looks removing does not break it.

Can you set this similarly to LLVM_DEP_PACKAGES, so these are defined in one place.

i.e.

INTROSPECTOR_DEP_PACKAGES="texinfo bison flex"

jonathanmetzman · 2022-01-13T14:45:43Z

infra/helper.py

      'FUZZING_ENGINE=' + engine,
      'SANITIZER=' + sanitizer,
      'ARCHITECTURE=' + architecture,
+      'GITHUB_REPO=',  # TODO(navidem): fix this to be loaded through main_repo property in project.yaml.


Should this be called "GITHUB_REPO" instead of "GIT_REPO" ?
Is it really specific to github?

jonathanmetzman · 2022-01-13T14:46:28Z

infra/base-images/base-builder/compile

+  mv /usr/bin/ar /usr/bin/old-ar
+  mv /usr/bin/ranlib /usr/bin/old-ranlib
+
+  ln -s /usr/local/bin/llvm-ar /usr/bin/ar


Hmmm...Also, is this script idempotent. Might be a good idea to make it so to help with debugging.

…ector

Navidem · 2022-01-20T00:43:34Z

@oliverchang This should be ready for merge.

jonathanmetzman · 2022-01-28T14:44:21Z

infra/base-images/base-builder/Dockerfile

 COPY precompile_afl /usr/local/bin/
 RUN precompile_afl

+RUN git clone https://github.com/ossf/fuzz-introspector.git fuzz-introspector && \


Why do we need to clone this again if it's already been cloned in base-clang?
I think putting this in /src is causing breakages.
Although currently we (oss-fuzz's maintainers) use /src, we probably shouldn't.

addressed in #7199

jonathanmetzman · 2022-01-28T14:45:34Z

infra/base-images/base-builder/compile

+  export AR=llvm-ar
+  export RANLIB=llvm-ranlib
+
+  # Move ar and ranlib


nit: this comment should have a period.
More importantly, I think it isn't a useful comment and I would change. It's clear that ar and ranlib are being moved, it's not clear why, a better comment would explain this.

infra/base-images/base-builder/compile

jonathanmetzman · 2022-01-28T14:46:33Z

infra/base-images/base-clang/Dockerfile


 FROM gcr.io/oss-fuzz-base/base-image

+ARG introspector


I wonder if putting this so high up impacts caching. My first guess is that it won't but I think it's worth testing.

jonathanmetzman · 2022-01-28T14:50:22Z

infra/base-images/base-clang/Dockerfile

 # Install newer cmake.
 ENV CMAKE_VERSION 3.21.1
-RUN apt-get update && apt-get install -y wget sudo && \
+RUN apt-get update && apt-get install -y wget sudo git && \


I don't really love the placement of this install. This line is meant to install cmake and deletes the packages used to build it., I think instead, you should install git on line 34 and then uninstall it in the same step.
I think we should uninstall it there because it will just add bloat to the image size since in checkout_build_install_llvm.sh we install git and then uninstall it.

addressed in #7199

infra/base-images/base-clang/checkout_build_install_llvm.sh

jonathanmetzman · 2022-01-28T14:51:38Z

infra/base-images/base-clang/checkout_build_install_llvm.sh

 echo "Using LLVM revision: $LLVM_REVISION"

+if [ -n "$INTROSPECTOR_PATCHES" ]; then
+  # For fuzz introspector.


Maybe this comment would be improved by explaining what this section does for introspector, since that part is hard to understand.

jonathanmetzman · 2022-01-28T14:52:15Z

infra/base-images/base-clang/checkout_build_install_llvm.sh

+if [ -n "$INTROSPECTOR_PATCHES" ]; then
+  # For fuzz introspector.
+  echo "Applying introspector changes"
+  BBBASE=$PWD


Maybe call this variable OLD_WORKING_DIR instead of BBBASE

addressed in #7199

oliverchang · 2022-01-31T03:55:31Z

Thanks @jonathanmetzman ! @Navidem please address these comments in another PR.

apply patches needed for fuzz introspector integration

8e0c74f

Navidem requested review from jonathanmetzman and oliverchang January 11, 2022 21:47

DavidKorczynski reviewed Jan 11, 2022

View reviewed changes

infra/base-images/base-builder/Dockerfile Outdated Show resolved Hide resolved

infra/base-images/base-builder/Dockerfile Outdated Show resolved Hide resolved

use correct lto flags and pin to later commit

1496d04

oliverchang reviewed Jan 12, 2022

View reviewed changes

jonathanmetzman reviewed Jan 12, 2022

View reviewed changes

DavidKorczynski mentioned this pull request Jan 13, 2022

Add build steps for introspector images #7059

Merged

applied the comments and made changes directly in bsae-clang

dfd61f4

oliverchang approved these changes Jan 13, 2022

View reviewed changes

Fix presubmit issue

1f31949

oliverchang approved these changes Jan 14, 2022

View reviewed changes

jonathanmetzman reviewed Jan 14, 2022

View reviewed changes

Made compile script idempotent

bed4ad3

oliverchang mentioned this pull request Jan 19, 2022

Integrate Fuzz Introspector with coverage build #7060

Closed

Navidem added 4 commits January 18, 2022 18:38

Removing extra packages and rename git_repo

ba86c74

Fix git repo env in compile script

d6d4fa7

Pass coverage url in compile script

77c8832

Bumping introspector checkout, and using list of packages for introsp…

0a5f721

…ector

oliverchang approved these changes Jan 20, 2022

View reviewed changes

oliverchang merged commit 9e39d35 into master Jan 20, 2022

oliverchang deleted the patch_form_FI branch January 20, 2022 01:22

jonathanmetzman reviewed Jan 28, 2022

View reviewed changes

MartinPetkov pushed a commit to MartinPetkov/oss-fuzz that referenced this pull request Aug 15, 2022

apply patches needed for fuzz introspector integration (google#7122)

fdeb206

Conversation

Navidem commented Jan 11, 2022

Uh oh!

DavidKorczynski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

oliverchang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavidKorczynski commented Jan 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oliverchang left a comment

Choose a reason for hiding this comment

Uh oh!

oliverchang commented Jan 13, 2022

Uh oh!

oliverchang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

DavidKorczynski commented Jan 12, 2022 •

edited

Loading