Repository navigation
OSS-Fuzz: Move fuzzer to upstream repository - #1724
arthurscchan wants to merge 5 commits into
Conversation
Signed-off-by: Arthur SC Chan <arthur.chan@adalogics.com>
 [Low impact] The changes reach only opt-in fuzzing and OSS-Fuzz build workflows. Fix the object-only library selection before merging. Findings
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Signed-off-by: Arthur SC Chan <arthur.chan@adalogics.com>
Signed-off-by: Arthur SC Chan <arthur.chan@adalogics.com>
Coverage Report for CI Build 39566034341Coverage remained the same at 89.907%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
baylesj
left a comment
baylesj
left a comment
There was a problem hiding this comment.
Thanks for addressing the automatic greptile feedback. I did a more extensive review and have some asks before landing this in JsonCpp.
| @@ -0,0 +1,158 @@ | |||
| // Copyright 2020 Google Inc. | |||
There was a problem hiding this comment.
I'm not morally opposed to landing this PR, however introducing Apache licensed code has some licensing headaches that I would prefer to avoid. Can you relicense this as a new file matching JsonCpp's existing licensing? it is allowed under Google policy AFAIK.
| # sources are kept here so they live with the other jsoncpp fuzzers. | ||
| set(JSONCPP_FUZZ_ENGINE "" CACHE STRING | ||
| "Fuzzing engine link flags for jsoncpp_fuzzer, e.g. -fsanitize=fuzzer") | ||
| if(JSONCPP_WITH_FUZZING) |
There was a problem hiding this comment.
Nothing builds this target yet, since the OSS-Fuzz build.sh still compiles fuzz.cpp directly. Also, with this ON and JSONCPP_FUZZ_ENGINE empty, linking fails because fuzz.cpp has no main. Could we error out at configure time here (or require the engine flags)?
| # main() is not pulled into compiler checks and other executables. | ||
| # | ||
| # The libprotobuf-mutator based fuzzer (jsoncpp_fuzz_proto.cc, json.proto, | ||
| # json_proto_converter.*) is built by OSS-Fuzz separately with LPM; the |
There was a problem hiding this comment.
If the sources live here, I'd want CI to build them; otherwise they'll only break in OSS-Fuzz. A CIFuzz workflow would cover both fuzzers.
There was a problem hiding this comment.
I can probably handle this as a follow up if needed.
| required JsonObject object_value = 2; | ||
| } | ||
|
|
||
| message JsonObject { |
There was a problem hiding this comment.
Since JsonObject has exactly one name/value, generated objects never have more than one member, so rejectDupKeys can never trigger and object-member handling barely gets exercised. JsonObject.settings is also unused.
|
|
||
| #include "json_proto_converter.h" | ||
|
|
||
| #include <charconv> |
There was a problem hiding this comment.
Floating-point std::to_chars needs C++17 and a fairly recent standard library (libc++ 14+, libstdc++ 11+). The library itself is C++11, and this only builds in OSS-Fuzz because build.sh seds the standard to 17.
| if (number_value.has_float_value()) { | ||
| const double value = number_value.float_value().value(); | ||
| // Keep "inf"/"nan" out of the output; they are not valid JSON numbers. | ||
| data_ << (std::isfinite(value) ? value : 0.0); |
There was a problem hiding this comment.
This still writes doubles at the stream's default precision of 6 significant digits, which is the same digit loss that was fixed for exponent_frac_value above.
| Json::Value root; | ||
| try { | ||
| reader->parse(begin, end, &root, nullptr); | ||
| } catch (Json::Exception const&) { |
There was a problem hiding this comment.
nit: fuzz.cpp wraps this in #if JSON_USE_EXCEPTION so it builds with exceptions disabled. Also, the Json::Exception forward declaration and the duplicate or unused includes above can go.
|
|
||
| } // namespace json_proto | ||
|
|
||
| #endif // TESTING_LIBFUZZER_PROTO_JSON_PROTO_CONVERTER_H_ |
There was a problem hiding this comment.
nit: the comment doesn't match the guard name (JSON_PROTO_CONVERTER_H_).
| if(BUILD_SHARED_LIBS) | ||
| target_link_libraries(jsoncpp_fuzzer jsoncpp_lib) | ||
| else() | ||
| target_link_libraries(jsoncpp_fuzzer jsoncpp_static) |
There was a problem hiding this comment.
With BUILD_SHARED_LIBS=OFF, BUILD_STATIC_LIBS=OFF, BUILD_OBJECT_LIBS=ON, and tests disabled, enabling JSONCPP_WITH_FUZZING selects jsoncpp_static, which is never created. The fuzz target then fails to link even when a fuzzing engine is supplied. Select jsoncpp_object when only the object library is enabled.
Knowledge Base Used: CMake, Meson, and Bazel integration
|
Thanks for the comments. Please spare me a few days to address those comments as I am stucked in some other things now. I will address them asap. Sorry for the delay. |
This PR moves all the OSS-Fuzz fuzzers to the upstream storage for easier management. This PR works together with OSS-Fuzz (google/oss-fuzz#16308) to accommodate the moving of fuzzers upstream.
Of course, this is only a suggestion. If the maintainers would prefer the fuzzers to remain in OSS-Fuzz, I am happy to keep the current setup and to ping the relevant maintainers for review whenever I add new fuzzers later in the process.
Thank you for considering this, and I would welcome any feedback on the preferred approach.
Remark: After this PR is accepted and merged, there will be additional PRs for adding new fuzzers for this project later.