FuzzIntrospector build steps by Navidem · Pull Request #7162 · google/oss-fuzz

Navidem · 2022-01-20T01:11:23Z

Adds build steps for fuzz introspector to the coverage build.

jonathanmetzman

This looks much much better! Nice work

jonathanmetzman · 2022-01-20T02:09:18Z

infra/build/functions/build_and_run_coverage.py

 class Bucket:  # pylint: disable=too-few-public-methods
-  """Class representing the coverage GCS bucket."""
+  """Class representing the GCS bucket."""



I think you should probably define a BUCKET_NAME = None here.
@oliverchang WDYR?
On the one hand not doing None makes incorrect implementations fail louder, on the other hand it seems too-dynamic and icky to do this.

The other option would be to override init and use super.

Yes, the BUCKET_NAME = None was needed for lint.

jonathanmetzman · 2022-01-20T02:11:20Z

infra/build/functions/build_and_run_coverage.py

+  introspector_enabled = False
+  if project.fuzzing_language in LANGUAGES_WITH_INTROSPECTOR_SUPPORT:
+    introspector_enabled = True
+


Hmmm... maybe do

else introspector_enabled = False

got rid of this.

jonathanmetzman · 2022-01-20T02:11:52Z

infra/build/functions/build_and_run_coverage.py

+  runner_image_name = build_project.get_runner_image_name(
+      base_images_project, config.test_image_suffix)
+  if introspector_enabled:
+    runner_image_name += ':introspector'


Sorry, why are we using the tag for this instead of a different image name?

Got rid of this as well. We can use :latest now.

jonathanmetzman · 2022-01-20T02:15:38Z

infra/build/functions/build_and_run_coverage.py

          upload_fuzzer_stats_url,
      'html_report_url':
-          bucket.html_report_url,
+          os.path.join(bucket.html_report_url, 'index.html'),


I don't know what's the right thing to do here, but for URLs I generally use posixpath.join instead of os.path.join
since os.path.join would break on Windows for URLs (it would use backslash: "")

jonathanmetzman · 2022-01-20T17:10:26Z

infra/build/functions/build_and_run_coverage.py

                                 LATEST_REPORT_INFO_CONTENT_TYPE))
+
+  if introspector_enabled:
+    coverage_url = bucket.html_report_url


nit: Don't put this in a variable.

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

infra/build/functions/build_and_run_coverage.py

+                                              config.test_image_suffix),
+      'args': [
+          'bash', '-c',
+          ('sed -i s/base-builder/base-builder:introspector/g '


So what you can do to avoid this is the following
run the command:
docker tag gcr.io/oss-fuzz-base/base-builder:introspector gcr.io/oss-fuzz-base/base-builder:latest
and then you won't need sed.

Note that your technique and mine breaks if someone were to change the FROM line.
This happens sometimes when someone wants to use an older version of clang.

+1. Do this instead of sed.

jonathanmetzman · 2022-01-20T17:13:16Z

infra/build/functions/build_and_run_coverage.py

+          'bash', '-c',
+          ('sed -i s/base-builder/base-builder:introspector/g '
+           f'oss-fuzz/projects/{project.name}/Dockerfile'
+           f' && cat oss-fuzz/projects/{project.name}/Dockerfile')


This cat step shouldn't be necessary right?

OOPS! it was for debugging purpose.

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

infra/build/functions/build_and_run_coverage.py

+          'build',
+          '-t',
+          f'gcr.io/oss-fuzz/{project.name}',
+          '.',


Maybe instead of using "." and dir, you can use docker's --file argument.

oliverchang · 2022-01-21T01:34:32Z

infra/build/functions/build_and_run_coverage.py

+                                              config.test_image_suffix),
+      'args': [
+          'bash', '-c',
+          ('sed -i s/base-builder/base-builder:introspector/g '


+1. Do this instead of sed.

infra/build/functions/build_and_run_coverage.py

Navidem · 2022-01-24T23:32:15Z

I have to fix test failures.

oliverchang · 2022-01-24T23:54:47Z

infra/build/functions/deploy.sh

Nit: delete these empty lines

oliverchang · 2022-01-24T23:57:10Z

infra/build/functions/deploy.sh


+
+deploy_cloud_function request-introspector-build \
+						introspector_build \


nit: fix indent.

Think you forgot to push. I edited this file.

oliverchang · 2022-01-24T23:57:31Z

infra/build/functions/main.py

+
+def introspector_build(event, context):
+  """Entry point for cloud function to build introspector reports."""
+  request_introspector_build.request_introspector_build(event, context)


You need to add this file too (request_introspector_build.py).

Ah! done...

jonathanmetzman · 2022-01-25T04:17:48Z

infra/build/functions/main.py

 import request_introspector_build


-


tip: try not to make changes that aren't necessary for your PR, like deleting lines such as this one.

jonathanmetzman · 2022-01-25T04:17:56Z

infra/build/functions/request_introspector_build.py

@@ -0,0 +1,58 @@
+# Copyright 2022 Google Inc.


jonathanmetzman · 2022-01-25T04:19:45Z

infra/build/functions/build_and_run_coverage.py

          upload_fuzzer_stats_url,
      'html_report_url':
-          bucket.html_report_url,
+          os.path.join(bucket.html_report_url, 'index.html'),


jonathanmetzman · 2022-01-25T04:21:03Z

infra/build/functions/build_and_run_coverage.py

+def get_fuzz_introspector_steps(  # pylint: disable=too-many-locals, too-many-arguments, unused-argument
+    project_name, project_yaml_contents, dockerfile_lines, image_project,
+    base_images_project, config):
+  """Return build steps of fuzz introspector for project"""


I think the docstring should be in descriptive style acording to the style guide: https://google.github.io/styleguide/pyguide.html#383-functions-and-methods

e.g. "Returns build step" instead of "Return build step"

jonathanmetzman · 2022-01-25T04:26:00Z

infra/build/functions/build_lib.py

  return steps


+def download_coverage_data_steps(project_name, latest, bucket_name, testing):


I'm going to try to think of a way to share code with download_corpora_steps since it looks like this code was copied.

jonathanmetzman · 2022-01-25T04:26:59Z

infra/build/functions/build_lib.py

+  steps = []
+  fuzz_targets = _get_targets_list(project_name, testing)
+  if not fuzz_targets:
+    sys.stderr.write('No fuzz targets found for project "%s".\n' % project_name)


Can you log instead of writing to stderr please?

The other functions in build_lib.py are writing to stderr, too. Should we change them all?

Eh can leave for consistency. We can change them all another time.

oliverchang

Nice! Looking close! Just some more minor comments.

Also, we need tests for the request_introspector_build part. In the interests of time, we can do this in another PR (please add a TODO) if you've manually tested this already.

oliverchang · 2022-01-27T21:51:57Z

infra/build/functions/build_and_run_coverage.py

+
+  coverage_report_latest = build_project.get_datetime_yesterday().strftime(
+      '%Y%m%d')
+  # TODO: debug


Still relevant?

oliverchang · 2022-01-27T21:53:00Z

infra/build/functions/build_project.py

  return datetime.datetime.now()


+def get_datetime_yesterday():


Rather than this, we could also try to figure out the actual latest uploaded report by looking at the bucket. Can leave this as a TODO.

yes that is what we have to change later. For now I changed it to use the current date. It means it needs the coverage build have to finish before starting introspector build.

oliverchang · 2022-01-27T21:54:41Z

infra/build/functions/build_and_run_coverage.py

+          f'gcr.io/oss-fuzz/{project.name}',
+          '.',
+      ],
+      'dir': f'oss-fuzz/projects/{project.name}',


Use posixpath.join instead

I thought we use posixpath.join for URLs. Did you mean using os.path.join()?

Better to use posixpath here, as the Cloud Build environment is posix. The environment this script runs may not be posix.

Adds build steps for fuzz introspector to the coverage build.

Navidem added 7 commits January 18, 2022 18:35

Coverage and Introspector Bucket classes

e439eda

Added introspect steps up to compile

f40dce3

Copying the reports to introspector bucket

706d203

Set git repo and cov url env variables

abe5356

Adjust coverage url

20dcd64

Changing the url and classes

719342d

Adjust for infra-test

f7fb324

Navidem requested review from jonathanmetzman and oliverchang January 20, 2022 01:11

Navidem mentioned this pull request Jan 20, 2022

Integrate Fuzz Introspector with coverage build #7060

Closed

jonathanmetzman reviewed Jan 20, 2022

View reviewed changes

Navidem added 2 commits January 20, 2022 09:20

Once the patch is upstream, we can use base-runner:latest

88a0777

Fixing lint and other nits

c03f298

oliverchang reviewed Jan 21, 2022

View reviewed changes

Navidem added 2 commits January 24, 2022 14:51

Separate introspector build steps

3df8af6

Updating pubsub for introspector build

928cbf1

Merge branch 'master' into FI_build_steps

dee026f

oliverchang reviewed Jan 24, 2022

View reviewed changes

Navidem added 2 commits January 24, 2022 20:15

Nits and adding request_introspector_build

f554bc4

fix lint

ad2d189

jonathanmetzman reviewed Jan 25, 2022

View reviewed changes

Navidem and others added 6 commits January 26, 2022 11:18

Fixing some nits

0d69bbb

Do not ignore coverage build return value

6c62d4f

Merge remote-tracking branch 'origin/master' into FI_build_steps

1a83e56

Fxing too long lines

75b2af8

Adjusting to the new dir naming for storing text coverage reports

0451228

Updating expected steps to pass test

52f74e5

oliverchang reviewed Jan 27, 2022

View reviewed changes

Using current date for coverage report and other nits

2bb5ee8

Update deploy.sh

3c0bd58

oliverchang approved these changes Jan 31, 2022

View reviewed changes

oliverchang merged commit 42880c1 into master Jan 31, 2022

oliverchang deleted the FI_build_steps branch January 31, 2022 00:56

DonggeLiu pushed a commit that referenced this pull request Feb 3, 2022

FuzzIntrospector build steps (#7162)

ae51a39

Adds build steps for fuzz introspector to the coverage build.

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

FuzzIntrospector build steps (google#7162)

3129c4e

Adds build steps for fuzz introspector to the coverage build.



		deploy_cloud_function request-introspector-build \
		introspector_build \

		return steps


		def download_coverage_data_steps(project_name, latest, bucket_name, testing):

Conversation

Navidem commented Jan 20, 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!

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!

Uh oh!

Navidem commented Jan 24, 2022

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!

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

Navidem Jan 28, 2022 •

edited

Loading