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 Exiv2 #6186

Merged
merged 6 commits into from
Aug 17, 2021
Merged

Add Exiv2 #6186

merged 6 commits into from
Aug 17, 2021

Conversation

kevinbackhouse
Copy link
Contributor

Exiv2 is a command-line utility and C++ library for reading, writing, deleting, and modifying the metadata of image files. It would be great if you could add it to OSS-Fuzz!

I have fixed all the ASAN/UBSAN bugs that I am aware of. However, there are a few results like this, which I consider to be false positives:

/home/kev/exiv2/src/tiffvisitor_int.cpp:778:29: runtime error: null pointer passed as argument 2, which is declared to never be null
/usr/include/string.h:44:28: note: nonnull attribute specified here
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /home/kev/exiv2_0.27/src/tiffvisitor_int.cpp:778:29 in 

That error is due to calling memcpy() with a null pointer, but the size argument is zero, so I don't think it's a bug. The issue is discussed here on stackoverflow. Will that be a problem on OSS-Fuzz? If so, I will postpone this pull request until I have fixed those issues. (I am planning to do so anyway, but I am keen to get Exiv2 enrolled in OSS-Fuzz as soon as possible.)

@google-cla
Copy link

google-cla bot commented Aug 9, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@kevinbackhouse
Copy link
Contributor Author

@googlebot I signed it!

@kevinbackhouse kevinbackhouse marked this pull request as ready for review August 11, 2021 21:12
@kevinbackhouse
Copy link
Contributor Author

Please could you try running the checks again? I have made some changes and tested locally using these instructions.

@jonathanmetzman
Copy link
Contributor

Done.

@kevinbackhouse
Copy link
Contributor Author

I dropped AFL and honggfuzz, so the checks pass now. (I tested it on my own fork.) In the previous commit, I got the AFL build to pass by changing the way that I create the seed corpus, but honggfuzz was still failing with the message "ERROR: 100.0% of fuzz targets seem to be broken". Our fuzz target uses LLVMFuzzerTestOneInput, so our cmake build uses -fsanitize=fuzzer-no-link and -fsanitize=fuzzer. I guess those are libFuzzer features that shouldn't be used with AFL and honggfuzz, but I am not sure how to build the fuzzing target without them (there would be no main function). So the easiest solution is to drop AFL and honggfuzz, but please let me know if you have any advice on how to make our build system compatible with them.

@inferno-chromium
Copy link
Collaborator

I dropped AFL and honggfuzz, so the checks pass now. (I tested it on my own fork.) In the previous commit, I got the AFL build to pass by changing the way that I create the seed corpus, but honggfuzz was still failing with the message "ERROR: 100.0% of fuzz targets seem to be broken". Our fuzz target uses LLVMFuzzerTestOneInput, so our cmake build uses -fsanitize=fuzzer-no-link and -fsanitize=fuzzer. I guess those are libFuzzer features that shouldn't be used with AFL and honggfuzz, but I am not sure how to build the fuzzing target without them (there would be no main function). So the easiest solution is to drop AFL and honggfuzz, but please let me know if you have any advice on how to make our build system compatible with them.

OSS-Fuzz passes the right cflags and cxxflags when needing to build afl++ and honggfuzz, can you avoid setting those when run in OSS-Fuzz build environment ? AFL++ coverage is important as it finds its unique set of bugs.

@kevinbackhouse
Copy link
Contributor Author

@inferno-chromium: If I remove the -fsanitize=fuzzer flag from our cmake config then all the builds fail with the error "undefined reference to main" (link to test PR). That's because our fuzz target only defines LLVMFuzzerTestOneInput, and the main function is auto-generated by libFuzzer.

Is there a way to add a main function that is compatible with libFuzzer, AFL, and honggfuzz?

@kevinbackhouse
Copy link
Contributor Author

Ok, apparently the solution is to add $LIB_FUZZING_ENGINE as a linker flag (link to test PR).

I have created a pull request to update our cmake configuration. When that's merged, I'll add another commit to this PR to update build.sh. I'll switch this PR to draft until that's ready.

@kevinbackhouse kevinbackhouse marked this pull request as draft August 15, 2021 12:49
@jonathanmetzman
Copy link
Contributor

Ok, apparently the solution is to add $LIB_FUZZING_ENGINE as a linker flag (link to test PR).

I have created a pull request to update our cmake configuration. When that's merged, I'll add another commit to this PR to update build.sh. I'll switch this PR to draft until that's ready.

Right, the main function for AFL/LibFuzzer/HF is linked in via LIB_FUZZING_ENGINE

@kevinbackhouse kevinbackhouse marked this pull request as ready for review August 17, 2021 12:56
@kevinbackhouse
Copy link
Contributor Author

This should pass the automated checks now (link to test PR).

@inferno-chromium inferno-chromium merged commit c0b2a00 into google:master Aug 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants