Repository navigation
OSS-Fuzz: Move fuzzers to upstream repository - #1343
arthurscchan wants to merge 1 commit into
Conversation
Signed-off-by: Arthur Chan <arthur.chan@adalogics.com>
There was a problem hiding this comment.
Code Review
This pull request introduces libFuzzer-based fuzz targets for the C API of libjsonnet, including CMake configuration updates and documentation. The feedback highlights a potential CMake configuration error when building fuzzers without other binary targets, and recommends adding an explicit compiler check for Clang to prevent build failures with other compilers. Additionally, the reviewer points out that setting *buf = NULL on import failure in the fuzzers violates the JsonnetImportCallback contract and suggests allocating a dummy error message using jsonnet_realloc to avoid undefined behavior or false-positive crashes.
| if(BUILD_FUZZERS) | ||
| add_subdirectory(fuzz) | ||
| endif() |
There was a problem hiding this comment.
If BUILD_FUZZERS is enabled but other binary/library targets (such as BUILD_JSONNET, BUILD_TESTS, BUILD_STATIC_LIBS, etc.) are disabled, the required library target (jsonnet_static or jsonnet_shared) will not be defined. However, jsonnet_lib_for_binaries will still attempt to alias it, leading to a CMake configuration error.\n\nTo fix this, please update the conditions for building jsonnet_static and jsonnet_shared in the main CMakeLists.txt to also check for BUILD_FUZZERS. For example:\n- For static library (around line 213):\n if(BUILD_STATIC_LIBS OR (BUILD_SOME_BINARIES AND NOT BUILD_SHARED_BINARIES) OR (BUILD_FUZZERS AND NOT BUILD_SHARED_BINARIES))\n- For shared library (around line 231):\n if(BUILD_SHARED_LIBS OR (BUILD_SOME_BINARIES AND BUILD_SHARED_BINARIES) OR (BUILD_FUZZERS AND BUILD_SHARED_BINARIES))
| int ImportCallback(void* ctx, const char* base, const char* rel, | ||
| char** found_here, char **buf, size_t *buflen) { | ||
| // Don't load file and mark it as failure. | ||
| *buf = NULL; | ||
| *buflen = 0; | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
Setting *buf = NULL on import failure violates the JsonnetImportCallback contract, which expects an allocated error message in *buf when returning 1. Passing nullptr to std::string constructors or other library internals that expect a valid buffer can cause undefined behavior or crashes. To prevent false-positive crashes in OSS-Fuzz, allocate a dummy error message using jsonnet_realloc.
int ImportCallback(void* ctx, const char* base, const char* rel,\n char** found_here, char **buf, size_t *buflen) {\n // Don't load file and mark it as failure.\n JsonnetVm* jvm = static_cast<JsonnetVm*>(ctx);\n const char* msg = "Imports are disabled";\n size_t len = std::strlen(msg);\n *buf = jsonnet_realloc(jvm, nullptr, len);\n if (*buf != nullptr) {\n std::memcpy(*buf, msg, len);\n }\n *buflen = len;\n return 1;\n}| int ImportCallback(void* ctx, const char* base, const char* rel, | ||
| char** found_here, char **buf, size_t *buflen) { | ||
| // Don't load file and mark it as failure. | ||
| *buf = NULL; | ||
| *buflen = 0; | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
Setting *buf = NULL on import failure violates the JsonnetImportCallback contract, which expects an allocated error message in *buf when returning 1. Passing nullptr to std::string constructors or other library internals that expect a valid buffer can cause undefined behavior or crashes. To prevent false-positive crashes in OSS-Fuzz, allocate a dummy error message using jsonnet_realloc.
int ImportCallback(void* ctx, const char* base, const char* rel,\n char** found_here, char **buf, size_t *buflen) {\n // Don't load file and mark it as failure.\n JsonnetVm* jvm = static_cast<JsonnetVm*>(ctx);\n const char* msg = "Imports are disabled";\n size_t len = std::strlen(msg);\n *buf = jsonnet_realloc(jvm, nullptr, len);\n if (*buf != nullptr) {\n std::memcpy(*buf, msg, len);\n }\n *buflen = len;\n return 1;\n}| int ImportCallback(void* ctx, const char* base, const char* rel, | ||
| char** found_here, char **buf, size_t *buflen) { | ||
| // Don't load file and mark it as failure. | ||
| *buf = NULL; | ||
| *buflen = 0; | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
Setting *buf = NULL on import failure violates the JsonnetImportCallback contract, which expects an allocated error message in *buf when returning 1. Passing nullptr to std::string constructors or other library internals that expect a valid buffer can cause undefined behavior or crashes. To prevent false-positive crashes in OSS-Fuzz, allocate a dummy error message using jsonnet_realloc.
int ImportCallback(void* ctx, const char* base, const char* rel,\n char** found_here, char **buf, size_t *buflen) {\n // Don't load file and mark it as failure.\n JsonnetVm* jvm = static_cast<JsonnetVm*>(ctx);\n const char* msg = "Imports are disabled";\n size_t len = std::strlen(msg);\n *buf = jsonnet_realloc(jvm, nullptr, len);\n if (*buf != nullptr) {\n std::memcpy(*buf, msg, len);\n }\n *buflen = len;\n return 1;\n}| # libFuzzer-based fuzz targets for jsonnet. | ||
| # Only built with -DBUILD_FUZZERS=ON and requires clang. See README.md | ||
| # in this directory. | ||
|
|
There was a problem hiding this comment.
Since the fuzz targets use Clang-specific sanitization flags (-fsanitize=fuzzer-no-link,address and -fsanitize=fuzzer,address), compiling with other compilers (like GCC or MSVC) will fail. Adding an explicit compiler check at the beginning of fuzz/CMakeLists.txt prevents cryptic compiler/linker errors and provides a clear error message to the user.
# libFuzzer-based fuzz targets for jsonnet.\n# Only built with -DBUILD_FUZZERS=ON and requires clang. See README.md\n# in this directory.\n\nif(NOT CMAKE_CXX_COMPILER_ID MATCHES "Clang")\n message(FATAL_ERROR "Building fuzzers requires the Clang compiler.")\nendif()
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#16312) 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.