Add build steps for introspector images by Navidem · Pull Request #7059 · google/oss-fuzz

Navidem · 2021-12-22T23:24:19Z

No description provided.

oliverchang

Nice! Just a general comment on the patching approach below.

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

infra/build/functions/base_images.py

 BASE_PROJECT = 'oss-fuzz-base'
 TAG_PREFIX = f'gcr.io/{BASE_PROJECT}/'
 MAJOR_VERSION = 'v1'
+INTROSPECTOR_VERSION = 'introspector'


nit: s/VERSION/TAG/ to be more precise.

oliverchang · 2022-01-06T00:33:10Z

infra/build/functions/base_images.py

+          'bash', '-c',
+          ('cd fuzz-introspector/ && cd oss_fuzz_integration/'
+           ' && sed -i \'s/\.\/infra\/base\-images\/all.sh/'
+           '#\.\/infra\/base\-images\/all.sh/\''


Let's upstream as many of the image diffs as possible to avoid this hacky and difficult to maintain patching. Could you create another PR to incorporate the changes in https://github.com/ossf/fuzz-introspector/blob/main/oss_fuzz_integration/oss-fuzz-patches.diff ?

We can guard certain things with env vars if necessary.

made all.sh conditional to skip it if in cloud build.

DavidKorczynski · 2022-01-06T18:28:30Z

I think we should consider that fuzz-introspector currently builds a custom Clang with a 2-line patch due to https://reviews.llvm.org/D77704 Please see the lines here: https://github.com/ossf/fuzz-introspector/blob/main/oss_fuzz_integration/oss-fuzz-patches.diff#L127-L129
The cleanest solution when integrating with OSS-Fuzz may be to get rid of those patch lines, which would enable us to have the fuzz-introspector image much smaller in size or simply include it in the base-builder image by default as introspector would only then be a small sized library. I recon that this would benefit future maintenance as well.

Another thought is the use of git-repo-url in fuzz-introspector. In fuzz-introspector, this is only used to create quick URLs to the source code of a given project. The goal was to mimic the URLs in clusterfuzz stack traces, but, to speed up dev I decided to just include a simple param to helper.py in the fuzz-introspector PoC oss-fuzz integration. It would perhaps be nicer to not use that argument to helper.py but instead use the logic from clusterfuzz to get the URLs.

oliverchang · 2022-01-10T05:59:12Z

I think we should consider that fuzz-introspector currently builds a custom Clang with a 2-line patch due to https://reviews.llvm.org/D77704 Please see the lines here: https://github.com/ossf/fuzz-introspector/blob/main/oss_fuzz_integration/oss-fuzz-patches.diff#L127-L129 The cleanest solution when integrating with OSS-Fuzz may be to get rid of those patch lines, which would enable us to have the fuzz-introspector image much smaller in size or simply include it in the base-builder image by default as introspector would only then be a small sized library. I recon that this would benefit future maintenance as well.

Yes it would be really great if we could get https://reviews.llvm.org/D77704 merged, or alternatively use a different mechanism for computing call graphs. Both of those will take time though, and we'd like to start being able to run this at scale on all of our OSS-Fuzz projects.

Another thought is the use of git-repo-url in fuzz-introspector. In fuzz-introspector, this is only used to create quick URLs to the source code of a given project. The goal was to mimic the URLs in clusterfuzz stack traces, but, to speed up dev I decided to just include a simple param to helper.py in the fuzz-introspector PoC oss-fuzz integration. It would perhaps be nicer to not use that argument to helper.py but instead use the logic from clusterfuzz to get the URLs.

Yep! I also suggested to @Navidem in #7060 (comment) to just use the main_repo property in project.yaml.

DavidKorczynski · 2022-01-11T18:18:05Z

@Navidem FYI this commit ossf/fuzz-introspector@c27b407 updates such that we no longer require a special version of LLVM in fuzz-introspector. Fuzz-introspector now works with latest oss-fuzz as well

oliverchang · 2022-01-11T21:54:21Z

@Navidem FYI this commit ossf/fuzz-introspector@c27b407 updates such that we no longer require a special version of LLVM in fuzz-introspector. Fuzz-introspector now works with latest oss-fuzz as well

Nice, thanks!! That makes it so much easier to integrate.

DavidKorczynski · 2022-01-11T21:58:03Z

@Navidem FYI this commit ossf/fuzz-introspector@c27b407 updates such that we no longer require a special version of LLVM in fuzz-introspector. Fuzz-introspector now works with latest oss-fuzz as well

Nice, thanks!! That makes it so much easier to integrate.

Np - notice we do still need to patch clang :/ i.e. before we needed LLVM 12 + patching. Now we only need patching.

oliverchang · 2022-01-13T00:52:50Z

@Navidem FYI this commit ossf/fuzz-introspector@c27b407 updates such that we no longer require a special version of LLVM in fuzz-introspector. Fuzz-introspector now works with latest oss-fuzz as well

Nice, thanks!! That makes it so much easier to integrate.

Np - notice we do still need to patch clang :/ i.e. before we needed LLVM 12 + patching. Now we only need patching.

@DavidKorczynski

Re the patches: Are the required changes self contained in a bunch of LLVM tools (i.e. ranlib, ar) such that we can just copy out these binaries into the main base-clang image to be used on demand for introspector runs together with vanilla clang? Or are there more widespread changes that make this hard to do?

DavidKorczynski · 2022-01-13T01:03:01Z

@Navidem FYI this commit ossf/fuzz-introspector@c27b407 updates such that we no longer require a special version of LLVM in fuzz-introspector. Fuzz-introspector now works with latest oss-fuzz as well

Nice, thanks!! That makes it so much easier to integrate.

Np - notice we do still need to patch clang :/ i.e. before we needed LLVM 12 + patching. Now we only need patching.

@DavidKorczynski

Re the patches: Are the required changes self contained in a bunch of LLVM tools (i.e. ranlib, ar) such that we can just copy out these binaries into the main base-clang image to be used on demand for introspector runs together with vanilla clang? Or are there more widespread changes that make this hard to do?

@oliverchang the patch is a single line in terms of code, and then a header file inclusion. See this comment I wrote in another PR ~~#7059 (comment)~~ #7122 (comment)

The patch is simply to add a line before here: https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/IPO/PassManagerBuilder.cpp#L997 and then we add the required plugin header as well.

The line we add is PM.add(createInspectorPass()); and the specific sed used is here: https://github.com/ossf/fuzz-introspector/blob/main/oss_fuzz_integration/oss-fuzz-patches.diff#L124-L128

oliverchang · 2022-01-13T03:02:15Z

@Navidem FYI this commit ossf/fuzz-introspector@c27b407 updates such that we no longer require a special version of LLVM in fuzz-introspector. Fuzz-introspector now works with latest oss-fuzz as well

Nice, thanks!! That makes it so much easier to integrate.

Np - notice we do still need to patch clang :/ i.e. before we needed LLVM 12 + patching. Now we only need patching.

@DavidKorczynski
Re the patches: Are the required changes self contained in a bunch of LLVM tools (i.e. ranlib, ar) such that we can just copy out these binaries into the main base-clang image to be used on demand for introspector runs together with vanilla clang? Or are there more widespread changes that make this hard to do?

@oliverchang the patch is a single line in terms of code, and then a header file inclusion. See this comment I wrote in another PR ~~#7059 (comment)~~ #7122 (comment)

The patch is simply to add a line before here: https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/IPO/PassManagerBuilder.cpp#L997 and then we add the required plugin header as well.

The line we add is PM.add(createInspectorPass()); and the specific sed used is here: https://github.com/ossf/fuzz-introspector/blob/main/oss_fuzz_integration/oss-fuzz-patches.diff#L124-L128

Thanks for explaining! Re just including this into base-clang, my one concern is that it's one more thing that may block us from doing clang upgrades in the future, even if the patch is fairly small. Are there any expectations around stability with the plugin interface that's being used for the inspector pass for future llvm updates?

In any case, it's probably safer to start with a separate image first for testing and move to a single image in the future. Perhaps we may even find another mechanism for generating the call graph that doesn't require LTO?

oliverchang · 2022-01-13T05:39:47Z

infra/build/functions/base_images.py

+          'gcr.io/oss-fuzz-base/base-runner',
+      'args': [
+          'bash', '-c',
+          (f'sed -i s/base-clang/base-clang:{INTROSPECTOR_TAG}/g'


let's set the replace pattern to 'base-clang:.*' in case the base-clang is already set to a tag.

First, probably you meant 'base-clang.*', right?
Second, the replacement is applied on vanilla oss-fuzz clone, so there won't be an explicit tag for the image used by base-builder. Unless you are assuming in future we may have such tagging.

oliverchang · 2022-01-13T05:40:13Z

infra/build/functions/base_images.py

+    steps.append({
+        'args': [
+            'build',
+            '--build-arg': build_arg,


Does passing --build-arg with an empty string work as expected (i.e. a no-op) ?

You're right, the empty string breaks the build, did not test it properly last night!
Probably better approach would be passing build-arg for both image builds, in case of base-clang:introspector it is consumed. For the case of base-builder:introspector it produces a benign warning, but build succeeds.

DavidKorczynski · 2022-01-13T10:53:38Z

Thanks for explaining! Re just including this into base-clang, my one concern is that it's one more thing that may block us from doing clang upgrades in the future, even if the patch is fairly small. Are there any expectations around stability with the plugin interface that's being used for the inspector pass for future llvm updates?

Atm the goal is to maintain a stable interface for OSS-Fuzz so we wont run into updating issues, but also enable non-oss-fuzz builds which may have more experimental features.

Another aspect that may make things unstable in the long-term is introducing new languages into fuzz-introspector.

In any case, it's probably safer to start with a separate image first for testing and move to a single image in the future. Perhaps we may even find another mechanism for generating the call graph that doesn't require LTO?

Sounds good and I agree that it's probably safer. It would also be nice to not alert users on failed introspector builds (at least in the beginning) and instead give us a chance of going through them to correct potential issues.

FYI I don't think LTO will be removed in the near future - we still get significant benefits from it. However, if the OSS-Fuzz integration shows it's a problem for many projects then we should reconsider. With that said, the only project I know of that does not go well with LTO atm is bitcoin-core. Other projects that have failed with LTO has been due to memory exhaustion and this can sometimes be fixed by constraining the build (e.g. from N processes to just a few). We track these issues here

oliverchang · 2022-01-14T01:35:40Z

infra/build/functions/base_images.py

+
+  introspector_steps = _get_introspector_base_images_steps(
+      INTROSPECTOR_BASE_IMAGES, tag_prefix)
+  intro_images = [


nit: s/intro/introspector/. Always better to be more explicit (and consistent) here.

oliverchang · 2022-01-14T01:43:42Z

Please also fix the lint issues.

Navidem · 2022-01-20T00:44:05Z

@oliverchang this should be ready for merge.

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

oliverchang reviewed Jan 6, 2022

View reviewed changes

DavidKorczynski mentioned this pull request Jan 6, 2022

Integrate Fuzz Introspector with coverage build #7060

Closed

Navidem added 4 commits January 12, 2022 15:15

Add build steps for introspector images

f108fe2

fix too long line issue

dfa02ba

renaming tags and skipping sed in patch script

53086d7

adjust image build to the upstreamed changes from fuzz introspector

efbb559

Navidem force-pushed the images_build branch from dc94b5a to efbb559 Compare January 12, 2022 23:16

Adjust to runtime build arg

49e6520

oliverchang reviewed Jan 13, 2022

View reviewed changes

DavidKorczynski mentioned this pull request Jan 13, 2022

apply patches needed for fuzz introspector integration #7122

Merged

Navidem added 3 commits January 13, 2022 14:22

Made build arg conditional and used wildcard for clang tag

14884f4

remove debugging code

87f4963

add final new line

0d1c58d

oliverchang approved these changes Jan 14, 2022

View reviewed changes

final nits

6db5b31

oliverchang approved these changes Jan 20, 2022

View reviewed changes

oliverchang merged commit 43bccdd into master Jan 20, 2022

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

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

Add build steps for introspector images (google#7059)

c4eb127

Conversation

Navidem commented Dec 22, 2021

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!

DavidKorczynski commented Jan 6, 2022

Uh oh!

oliverchang commented Jan 10, 2022

Uh oh!

DavidKorczynski commented Jan 11, 2022

Uh oh!

oliverchang commented Jan 11, 2022

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

oliverchang commented Jan 13, 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!

DavidKorczynski commented Jan 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oliverchang commented Jan 14, 2022

Uh oh!

Navidem commented Jan 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

DavidKorczynski commented Jan 11, 2022 •

edited

Loading

oliverchang commented Jan 13, 2022 •

edited

Loading

DavidKorczynski commented Jan 13, 2022 •

edited

Loading