oss-fuzz icon indicating copy to clipboard operation
oss-fuzz copied to clipboard

Git fuzzing: uncomment the existing and add new targets.

Open JarLob opened this issue 7 months ago • 6 comments

This pull request:

  • Uncomments fuzzing targets that were commented out
  • Adds the targets that were mentioned in the audit done by X41
  • Adds some new fuzzing targets
  • Fixes false positives in the build with memory sanitizer because zlib not being built with the sanitizer
  • Builds corpus from smaller files
  • Removes unnecessary memory allocations in the fuzzing harness
  • Fixes compiler warnings (const unsigned char)

JarLob avatar Jan 12 '24 20:01 JarLob

JarLob is a new contributor to projects/git. The PR must be approved by known contributors before it can be merged. The past contributors are: illia-v, arthurscchan, steadmon, Navidem, ahunt, inferno-chromium, devtty1er

github-actions[bot] avatar Jan 12 '24 20:01 github-actions[bot]

Oh, I didn't realize there is this file system restriction. Though it sounds like it should be easily fixable. I'll look into it next week.

JarLob avatar Jan 17 '24 17:01 JarLob

My idea is to make all the files in the /tmp. That should work, shouldn't it?

JarLob avatar Feb 08 '24 12:02 JarLob

My idea is to make all the files in the /tmp. That should work, shouldn't it?

No, we did this and there are some complications with the approach. I'll dig out the issues tomorrow.

DavidKorczynski avatar Feb 08 '24 23:02 DavidKorczynski

I couldn't find issues in the logs because I disabled the cmd fuzzers.

From memory, I think what happens when you change the working directory of the fuzzer to /tmp/ is that the fuzzer-monitoring environment will fail, e.g. missed calls to llvm-symbolizer in the event of crashes. Another problem was due to all logic failing related to lockfiles https://github.com/git/git/blob/master/lockfile.c -- this issue with lockfiles I tried to resolve but stopped at some point.

Finally, the logic in git itself has a ton of calls to die which aborts the process, and this is causing a lot of issues for the cmd fuzzers. For example, cmd-diff-fuzzer is running right now. However, this fuzzer is constantly running into aborts.

I'm happy to move forward if you want to try and get some of the cmd fuzzers running. If we get them to run proper they will be particularly valuable, however, could we start with enabling 1 or 2 of them and not all of them to void spam.

DavidKorczynski avatar Feb 09 '24 18:02 DavidKorczynski

Yes, the die() function is a major obstacle for fuzzing. This is why I added more places where it is commented out to the patch.

Ok, so I didn't do any changes to create the temporary files in /tmp since you said there are other issues because of that and left as it is.

I have left fuzz-pack-headers fuzz-pack-idx fuzz-commit-graph fuzz-cmd-diff as they were enabled before, added fuzz-credential-from-url-gently fuzz-date fuzz-parse-attr-line fuzz-url-decode-mem fuzz-url-end-with-slash because they are simple and do not create any files and enabled only one of the new fuzzers fuzz-cmd-bundle-verify which may cause issues or not.

JarLob avatar Feb 14 '24 01:02 JarLob

@DavidKorczynski ?

JarLob avatar Feb 21 '24 14:02 JarLob