Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CIFuzz #559

Closed
wants to merge 1 commit into from
Closed

Add CIFuzz #559

wants to merge 1 commit into from

Conversation

AdamKorcz
Copy link

Adds CIFuzz to libjpeg-turbos OSS-fuzz integration. This will run libjpeg-turbos fuzers for 600 seconds when a pull request is made.

Documentation of CIFuzz is here: https://google.github.io/oss-fuzz/getting-started/continuous-integration/

@dcommander
Copy link
Member

Doesn't OSS-Fuzz run periodically on the main branch? If so, would it not eventually catch a regression that was checked in?

@dcommander
Copy link
Member

(My question comes from a desire to understand why this isn't a duplication of effort. Is CIFuzz just doing less work than OSS-Fuzz but more frequently?)

@AdamKorcz
Copy link
Author

AdamKorcz commented Oct 26, 2021

Doesn't OSS-Fuzz run periodically on the main branch?

Yes, it does.

If so, would it not eventually catch a regression that was checked in?

Yes, it would.

Is CIFuzz just doing less work than OSS-Fuzz but more frequently?

I guess you could say that, but the benefit doesn't come from just running the fuzzers more frequently. A benefit of CIFuzz is to catch bugs before they even enter the code base. While OSS-fuzz (the continuous fuzzing part that you are mentioning) does run periodically, it catches bugs after these have been sitting in a pull request, reviewed, approved and merged. CIFuzz in contrast fuzzes the code before it is merged.

@dcommander
Copy link
Member

Is CIFuzz just doing less work than OSS-Fuzz but more frequently?

I guess you could say that, but the benefit doesn't come from just running the fuzzers more frequently. A benefit of CIFuzz is to catch bugs before they even enter the code base. While OSS-fuzz (the continuous fuzzing part that you are mentioning) does run periodically, it catches bugs after these have been sitting in a pull request, reviewed, approved and merged. CIFuzz helps to prevent new bugs from being introduced.

That's a weak argument for libjpeg-turbo, since it is not a community-developed project, and PRs are infrequent. (Generally speaking, it requires specialized domain expertise in order to contribute meaningfully to this project, and only I have commit access.) Pull requests are welcome, but they are never accepted at face value. I review every one of them personally and carefully, and I can think of only one instance (a trivial one-liner) in which I accepted the PR as-is. PRs are not considered part of the supported code base, and it should be understood that The libjpeg-turbo Project disavows any claims regarding the quality of submitted code until/unless the code can be thoroughly reviewed and integrated. That's why our CI system does not spin pre-release builds based on PRs. Given that:

  • Practically speaking, a PR will never be integrated as-is.
  • There is a chance that CIFuzz will miss some bugs that will later be caught by OSS-Fuzz.
  • I will always wait for OSS-Fuzz to test the latest commits before I publish an official release.
  • Pre-release builds are meant for testing and are not considered production-quality. (That is, if downstream consumers choose to use pre-release code, they are doing so at their own risk.)

I don't see much benefit to CIFuzz at the moment. The code velocity of this project is relatively low and will remain so for the foreseeable future due to lack of funding. I encourage people with domain expertise to engage with me on developing this code, but the reality is that few have, and when they do, they tend to engage for a brief period of time until their pet feature is integrated, then they mostly disappear. PRs rarely come from such developers. More often, PRs are half-baked attempts by users to work around some problem that they don't fully understand, so historically, most PRs are not accepted.

I am open to arguments regarding the benefits of CIFuzz for commits that I personally push, but I don't see any strong arguments there either except in terms of code velocity, since any issues that CIFuzz would catch would be eventually caught by OSS-Fuzz within a couple of days. (Security-conscious users would be ill-advised to consider pre-release code to be secure anyhow. Also, most security-conscious consumers of libjpeg-turbo, such as browser developers, maintain their own libjpeg-turbo code bases and usually only pull code from our upstream repository in response to critical bug fixes or major new releases.)

I'm not saying "no." I'm just saying that I'm not yet convinced.

@AdamKorcz
Copy link
Author

That is well received from my side. I suggest we close this one for now.

Do you have any thoughts on how fuzzing libjpeg-turbo in the CI could help the project? If not, no problem.

@dcommander
Copy link
Member

Do you have any thoughts on how fuzzing libjpeg-turbo in the CI could help the project? If not, no problem.

Not at the moment. If the project eventually becomes more of a group effort, then CIFuzz would be beneficial, but my business model precludes making it a group effort. (There is rarely even enough money to compensate my own labor, much less the labor required to manage multiple developers.) I feel that the only way to get more people involved full-time with libjpeg-turbo would be for a CPU-neutral and O/S-neutral company with a proven track record in open source development (Google, for instance, or Red Hat) to acquire it. Otherwise, the velocity of libjpeg-turbo will always be limited by the willingness of outside sponsors to pay for my labor to maintain and improve it. (I received a generous grant from Google earlier this year, but all of that money was exhausted by OSS-Fuzz integration/coverage improvements and the libjpeg-turbo 2.1 release. I'm already borrowing into 2022 against the only continuous source of funding our project has, which only pays for 8 hours/month of labor.) I am open to such an acquisition, and I feel that it would help take the project to the next level. However, I'm also not desperate to make such a move, and I would certainly not let the project out of my control without adequate compensation for the value I have built in it over the past 11 years.

Anyway, long-winded response to basically say: let's back burner this for now. I'm marking it as "revisit."

@dcommander
Copy link
Member

dcommander commented Oct 26, 2021

Two things that I think would be much more beneficial to libjpeg-turbo:

  1. improving the OSS-Fuzz interface so that it is very clear which commits have been fuzz-tested
  2. the ability to test multiple branches with OSS-Fuzz. (New features are always maintained in the dev branch, and they are only merged into the main branch when preparing a beta release.)

Actually, if CIFuzz would allow us to fuzz the dev branch, then that would be the "killer app" that justifies integrating it. I would prefer the ability to do full OSS-Fuzz runs on the dev branch, but running CIFuzz on it would be better than nothing.

@AdamKorcz
Copy link
Author

the ability to test multiple branches with OSS-Fuzz

This one should definitely be doable now. From a high level it could be solved by:

  1. First compiling the fuzzers with the current build but naming the fuzzers $FUZZER_from_master_branch or a similar name that would make it possible to identify the branch.
  2. After that you could checkout dev and repeat the build and ultimately name the fuzzers $FUZZER_from_dev_branch

It would require maintenance of two separate builds: One for master and one for dev. I suppose they would be identical though.

Following this procedure, both sets of fuzzers would run continuously by OSS-fuzz.

@dcommander
Copy link
Member

I don't quite understand the procedure you describe above. The main issue is that OSS-Fuzz builds only one Docker image from https://github.com/google/oss-fuzz/blob/master/projects/libjpeg-turbo/Dockerfile, and that Dockerfile specifies a git clone from the libjpeg-turbo main branch. Ideally, what I'd like is to be able to specify multiple branches in our project.yaml file and have each branch, if it exists (our dev branch doesn't exist unless unreleased feature code has been committed, i.e. unless we're actively working toward a major new release), be tested in a matrix with the specified architectures and sanitizers.

Barring that, could CIFuzz at least be used on arbitrary branches? Given that it can be used on PRs, I assume that the answer is "yes." If so, then there is some value to integrating it.

@AdamKorcz
Copy link
Author

I don't quite understand the procedure you describe above.

Sorry for the confusion. If you have a build that works on both the main branch and the dev branch, then I can set up a PR that allows you to fuzz both branches as well as toggle the dev-branch fuzzing on and off. CIFuzz is not needed for this.

@dcommander
Copy link
Member

I don't quite understand the procedure you describe above.

Sorry for the confusion. If you have a build that works on both the main branch and the dev branch, then I can set up a PR that allows you to fuzz both branches as well as toggle the dev-branch fuzzing on and off. CIFuzz is not needed for this.

That's not what I asked. I asked whether CIFuzz can be used to fuzz arbitrary branches. I don't need anyone to solve my problems for me. I just need to develop a sufficient understanding of the systems involved so that I can make my own informed decisions. I don't get paid a salary to futz with libjpeg-turbo, and our project funding is very limited, so sometimes it's preferable to partially solve a problem with very little effort rather than to fully solve a problem with a lot of effort. Without more straightforward support for multiple branches in OSS-Fuzz, it seems as if per-branch fuzzing targets would have a high maintenance cost. However, if CIFuzz can be used to test arbitrary branches independently without making any modifications to our OSS-Fuzz project directory, then it has a very low maintenance cost for most of the benefit.

@dcommander
Copy link
Member

@AdamKorcz I don't understand why you won't answer my simple question about CIFuzz. If the answer to the question is "yes", then this PR will be integrated, which is presumably what you want.

@AdamKorcz
Copy link
Author

Hi, sorry for the delay. Yes, CIFuzz can be set up to only run when changes are made on specific branches. The relevant part of the docs is here: https://google.github.io/oss-fuzz/getting-started/continuous-integration/#branches-and-paths

As I understand - and I may be wrong here - you are fundamentally interested in fuzzing the dev branch whether this happens in the CI or not. If that is the case, then I will just elaborate slightly on my last point which was that you don't need CIFuzz if the sole purpose is to fuzz two branches. This can be achieved by OSS-fuzz without introducing CIFuzz, and without changes to either the Dockerfile and the project.yaml file.

That being said, if you would like to run CIFuzz only when changes are made to the dev branch, then it sounds like CIFuzz will be useful.

@dcommander
Copy link
Member

As I understand - and I may be wrong here - you are fundamentally interested in fuzzing the dev branch whether this happens in the CI or not. If that is the case, then I will just elaborate slightly on my last point which was that you don't need CIFuzz if the sole purpose is to fuzz two branches. This can be achieved by OSS-fuzz without introducing CIFuzz, and without changes to either the Dockerfile and the project.yaml file.

I don't understand how that can be achieved without modifying the Dockerfile, since the Dockerfile specifies cloning libjpeg-turbo from the main branch. However, please submit a new PR so I can fully understand the ramifications. I want to have a complete understanding of that before I decide whether that approach or CIFuzz is the better approach for our project. OSS-Fuzz by itself would be fine if I could straightforwardly fuzz the dev branch with it.

dcommander added a commit that referenced this pull request Nov 19, 2021
CIFuzz runs the project's fuzzers for a limited period of time any time
a commit is pushed or a PR is submitted.  This is not intended to
replace OSS-Fuzz but rather to allow us to more quickly catch some
fuzzing failures, including fuzzer build regressions like the one
introduced in ecf021b.

Closes #559
@dcommander
Copy link
Member

After derping and accidentally pushing, then releasing (in libjpeg-turbo 2.1.2), a change that broke the build of one of the fuzzers, I see the wisdom of integrating this.

@dcommander dcommander reopened this Mar 28, 2022
@dcommander
Copy link
Member

@AdamKorcz I have now had the opportunity to run CIFuzz on both the dev and 2.0.x branches. Unfortunately, as the logs clearly show:
https://github.com/libjpeg-turbo/libjpeg-turbo/runs/5728485608
https://github.com/libjpeg-turbo/libjpeg-turbo/runs/5518437997
it is always pulling libjpeg-turbo from the main branch. This was specifically something I asked you how to solve, but you never provided a clear answer to my question regarding how to do that.

@dcommander
Copy link
Member

And no, I am not talking about how to make CIFuzz trigger on specific branches. I am talking about how to make it build the specific branch I am trying to test.

@dcommander
Copy link
Member

Referring to google/oss-fuzz#7479, I have apparently been misled in this PR regarding the limitations of CIFuzz. It will be un-integrated very soon.

@dcommander dcommander closed this Apr 6, 2022
dcommander added a commit that referenced this pull request Apr 6, 2022
Referring to the conversation in
google/oss-fuzz#7479 and #559, there was a
misunderstanding regarding how CIFuzz works.  It cannot be used to fuzz
arbitrary PRs or code branches, and it has a 90-day delay in downloading
corpora from OSS-Fuzz.  That makes it unsuitable for libjpeg-turbo.
dcommander added a commit that referenced this pull request Apr 6, 2022
Referring to the conversation in
google/oss-fuzz#7479 and #559, there was a
misunderstanding regarding how CIFuzz works.  It cannot be used to fuzz
arbitrary PRs or code branches, and it has a 90-day delay in downloading
corpora from OSS-Fuzz.  That makes it unsuitable for libjpeg-turbo.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants