Integrate Fuzz Introspector with coverage build by Navidem · Pull Request #7060 · google/oss-fuzz

Navidem · 2021-12-23T19:30:55Z

Added build steps to the coverage build to integrate fuzz intrsopsector.

oliverchang · 2022-01-06T00:34:27Z

infra/build/functions/build_and_run_coverage.py

+
+
+def get_fuzz_introspector_steps(project, project_name, base_images_project,
+                                config, coverage_url):


Please add a docstring.

oliverchang · 2022-01-06T00:34:54Z

infra/build/functions/build_and_run_coverage.py

+def get_fuzz_introspector_steps(project, project_name, base_images_project,
+                                config, coverage_url):
+  build_steps = []
+  FI_dir = '/workspace/fuzz-introspector/'


Use the full name. i.e. FUZZ_INTROSPECTOR_DIR, and put it above globally together with the other constants.

oliverchang · 2022-01-06T00:35:12Z

infra/build/functions/build_and_run_coverage.py

+                                config, coverage_url):
+  build_steps = []
+  FI_dir = '/workspace/fuzz-introspector/'
+  oss_integration_dir = 'oss_fuzz_integration/'


Also, let's not hardcode '/' in the directory constants.

Use os.path.join where needed. In this case though, we can skip the separator completely and just do

oss_fuzz_integration_dir = 'oss_fuzz_integration'

oliverchang · 2022-01-06T00:36:14Z

infra/build/functions/build_and_run_coverage.py

+  return build_steps
+
+
+def get_fuzz_introspector_steps(project, project_name, base_images_project,


Please write a test for this as well. See the existing build_and_run_coverage_test.py file for examples.

oliverchang · 2022-01-06T00:39:22Z

infra/build/functions/build_and_run_coverage.py

+      'args': [
+          'bash', '-c',
+          (f'cd {FI_dir} && cd {oss_integration_dir}'
+           ' && sed -i \'s/\.\/infra\/base\-images\/all.sh/#\.\/infra\/base\-images\/all.sh/\''


This is very hacky and fragile.

Let's upstream changes and do things in the main oss-fuzz repo where we can rather than using sed.

Also is this actually still needed when we are building the customized build images in the base images build function?

+++1.
Also I don't like that this is relying on all.sh. As far as I know all.sh is meant for local development.

oliverchang · 2022-01-06T00:40:23Z

infra/build/functions/build_and_run_coverage.py

+  #adjust coverage url
+  cov_url_escaped = coverage_url.replace("/", "\/").replace(":", "\:")
+  set_cov_url = (
+      f'sed -i \'s/http\:\/\/localhost\:8008\/covreport\/linux/{cov_url_escaped}/\''


Why is this needed? Good to explain with a comment in cases like this.

Also, is there a better way to do this that doesn't involve using sed?

oliverchang · 2022-01-06T00:40:47Z

infra/build/functions/build_project.py

      'HOME': '/root',
      'OUT': build.out,
  }
+


nit: remove unnecessary change.

oliverchang · 2022-01-06T00:40:57Z

infra/build/functions/build_project.py

 def get_compile_step(project, build, env, parallel):
  """Returns the GCB step for compiling |projects| fuzzers using |env|. The type
  of build is specified by |build|."""
+  set_git_repo_env = ''  #do nothing


no need for the comment here.

oliverchang · 2022-01-06T00:41:20Z

infra/build/functions/build_project.py

+  set_git_repo_env = ''  #do nothing
+  if build.sanitizer == 'instrumentor':
+    set_git_repo_env = (
+        ' && export GITHUB_REPO=$(grep -P -o "\S+github.com\S+" /workspace/oss-fuzz/projects/'


Can we do this in a less hacky way?

It's used by the helper.py script right?

We can instead modify the helper.py script to set this based on the main_repo property in project.yaml. e.g.

oss-fuzz/projects/alembic/project.yaml

Line 6 in 126cf11

main_repo: 'https://github.com/alembic/alembic'

. Let's do all this in another PR which upstreams the patches from fuzz introspector.

This is slightly different from how the existing patch does it: https://github.com/ossf/fuzz-introspector/blob/a49f0ca54103e6dc0177700d22a166a727683334/oss_fuzz_integration/oss-fuzz-patches.diff#L194 but it's less hacky.

oliverchang · 2022-01-06T00:49:12Z

infra/build/functions/build_and_run_coverage.py

                                 latest_report_info_url,
                                 LATEST_REPORT_INFO_CONTENT_TYPE))
+
+  #currently fuzz introspector only supports c and c++


add a space after all '#'

And start every comment with a capital letter and end them with punctuation marks.

oliverchang · 2022-01-06T00:49:17Z

infra/build/functions/build_and_run_coverage.py

+
+  #currently fuzz introspector only supports c and c++
+  if project.fuzzing_language in ['c', 'c++']:
+    #removes index.html from the end of url


oliverchang · 2022-01-06T00:49:31Z

infra/build/functions/build_and_run_coverage.py

+  #currently fuzz introspector only supports c and c++
+  if project.fuzzing_language in ['c', 'c++']:
+    #removes index.html from the end of url
+    coverage_url = bucket.html_report_url[:-11]


what is -11? either calculate this from a len(CONSTANT) or do a CONSTANT = 11

oliverchang · 2022-01-06T00:50:28Z

Also please fix the lint failures in https://github.com/google/oss-fuzz/runs/4622563959?check_suite_focus=true

jonathanmetzman · 2022-01-06T16:50:29Z

CI is failing because of test and presubmit failures. Please fix them.

jonathanmetzman

I think this PR needs some redesigning at a high-level before we proceed on fixing individual issues.
I'm most concerned with the interface between oss-fuzz and fuzz-introspector (as is, this code (which lives in oss-fuzz) clones fuzz-introspector, which clones oss-fuzz again.
I'm also concerned with the frequent use of sed here. I think it probably doens't need to be used at all.

jonathanmetzman · 2022-01-06T16:48:22Z

infra/build/functions/build_and_run_coverage.py

            f'/{upload_type}/{self.date}')


+class IntrospectorBucket:  # pylint: disable=too-few-public-methods


Let's make a BaseBucket class that Bucket (which should be named CoverageBucket) and IntrospectorBucket can inherit from (or maybe we don't need 3 classes, maybe we can just have a bucket class and set the attributes that need to be different. Either way as is, this isn't good, it's basically duplicated code.

jonathanmetzman · 2022-01-06T16:49:00Z

infra/build/functions/build_and_run_coverage.py

                                 latest_report_info_url,
                                 LATEST_REPORT_INFO_CONTENT_TYPE))
+
+  #currently fuzz introspector only supports c and c++


And start every comment with a capital letter and end them with punctuation marks.

jonathanmetzman · 2022-01-06T16:52:52Z

infra/build/functions/build_and_run_coverage.py


 # Where code coverage reports need to be uploaded to.
 COVERAGE_BUCKET_NAME = 'oss-fuzz-coverage'
+INTROSPECTOR_BUCKET_NAME = 'oss-fuzz-introspector'


Why are these constants necessary?
We set these strings to be equal to them on lines 53 and 84 anyway.
Reuse them on lines 53 and 84 please.

jonathanmetzman · 2022-01-06T16:53:23Z

infra/build/functions/build_and_run_coverage.py

@@ -36,6 +36,7 @@

 # Where code coverage reports need to be uploaded to.


This comment doesn't apply to the constant you added.

jonathanmetzman · 2022-01-06T16:56:53Z

infra/build/functions/build_and_run_coverage.py

-                                              config.test_image_suffix),
-      'env':
-          coverage_env,
+      'name': 'gcr.io/oss-fuzz-base/base-runner:introspector',


Why did you make this change?

Your change breaks testing, see how the actual function you get rid of is implemented

oss-fuzz/infra/build/functions/build_project.py

Line 441 in 316f788

def get_runner_image_name(base_images_project, test_image_suffix):

It just seems very weird that a tag is used for the introspector version instead of base-introspector.

Our current code allows changing oss-fuzz-base to something else (that's why base_images_project (defined here) is passed around. This breaks that. @oliverchang is this a feature worth keeping?

jonathanmetzman · 2022-01-06T17:20:12Z

infra/build/functions/build_and_run_coverage.py

+                                              config.test_image_suffix),
+      'args': [
+          'bash', '-c',
+          (f'cd {FI_dir} && cd {oss_integration_dir}'


This is very difficult to understand.
Instead of defining oss_integration_dir to implicitly be a subdir of FI_DIR make it an absolute path like so:

oss_integration_dir = os.path.join(FI_DIR, 'oss_fuzz_integration')

And then you will only have to cd once here.

jonathanmetzman · 2022-01-06T17:23:31Z

infra/build/functions/build_and_run_coverage.py

+                                config, coverage_url):
+  build_steps = []
+  FI_dir = '/workspace/fuzz-introspector/'
+  oss_integration_dir = 'oss_fuzz_integration/'


Could you explain what oss_integration_dir is in a comment?
I had to figure out what it is by going to the fuzz_introspector repo, can't expect readers to do this.

jonathanmetzman · 2022-01-06T17:23:53Z

infra/build/functions/build_and_run_coverage.py

+          (f'cd {FI_dir} && cd {oss_integration_dir}'
+           ' && sed -i \'s/\.\/infra\/base\-images\/all.sh/#\.\/infra\/base\-images\/all.sh/\''
+           ' build_patched_oss_fuzz.sh'
+           ' && ./build_patched_oss_fuzz.sh')


Where is this file?

jonathanmetzman · 2022-01-06T17:31:08Z

infra/build/functions/build_and_run_coverage.py

+           ' && ./build_patched_oss_fuzz.sh')
+      ]
+  })
+
+  build_steps.append({
+      'name':
+          build_project.get_runner_image_name(base_images_project,
+                                              config.test_image_suffix),
+      'args': [
+          'bash', '-c',
+          ('sed -i s/base-builder/base-builder:introspector/g '
+           f'{FI_dir}{oss_integration_dir}oss-fuzz/projects/{project_name}/Dockerfile'


OK, I think I figured out what this is doing and I think this isn't well designed.
If I understand correctly, the highlighted lines calls build_patched_oss_fuzz.sh from our cloned copy of fuzz-introspector which then clones oss-fuzz again.
I think before we continue with this PR we should go back to the drawing board and describe at a high-level how this should work and then implement based on that spec.

To follow on this, there are some other thoughts on fuzz-introspector that I think makes sense to consider when integrating fuzz-introspector to oss-fuzz: #7059 (comment)

jonathanmetzman · 2022-01-06T17:35:19Z

infra/build/functions/build_and_run_coverage.py

+          '-m',
+          'cp',
+          '-r',
+          os.path.join(build.out, 'inspector-tmp'),


What's inspector-tmp? Where does this come from?

Navidem · 2022-01-20T01:12:51Z

Thanks for all the comments, closing this as #7162 is the one to move forward with.

Integrate Fuzz Introspector with coverage build

0102947

Navidem mentioned this pull request Dec 23, 2021

Adding Fuzz Introspector to cloud build #7050

Closed

make introspector steps conditional on the fuzzer language

39655e7

Navidem requested a review from oliverchang January 5, 2022 22:46

oliverchang reviewed Jan 6, 2022

View reviewed changes

oliverchang requested a review from jonathanmetzman January 6, 2022 00:48

oliverchang reviewed Jan 6, 2022

View reviewed changes

jonathanmetzman requested changes Jan 6, 2022

View reviewed changes

oliverchang mentioned this pull request Jan 10, 2022

Add build steps for introspector images #7059

Merged

Navidem closed this Jan 20, 2022



		def get_fuzz_introspector_steps(project, project_name, base_images_project,
		config, coverage_url):

		return build_steps


		def get_fuzz_introspector_steps(project, project_name, base_images_project,

		f'/{upload_type}/{self.date}')


		class IntrospectorBucket: # pylint: disable=too-few-public-methods

		@@ -36,6 +36,7 @@

		# Where code coverage reports need to be uploaded to.

Conversation

Navidem commented Dec 23, 2021

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!

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 commented Jan 6, 2022

Uh oh!

jonathanmetzman commented Jan 6, 2022

Uh oh!

jonathanmetzman 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!

Choose a reason for hiding this comment

Uh oh!

Navidem commented Jan 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants