Skip to content

OSS-Fuzz: Move fuzzers to upstream repository - #1343

Open
arthurscchan wants to merge 1 commit into
google:masterfrom
arthurscchan:fuzz/upstream-harness
Open

arthurscchan wants to merge 1 commit into
google:masterfrom
arthurscchan:fuzz/upstream-harness

Conversation

@arthurscchan

Copy link
Copy Markdown

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.

Signed-off-by: Arthur Chan <arthur.chan@adalogics.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread CMakeLists.txt
Comment on lines +302 to +304
if(BUILD_FUZZERS)
add_subdirectory(fuzz)
endif()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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))

Comment on lines +25 to +31
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;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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}

Comment on lines +25 to +31
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;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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}

Comment on lines +25 to +31
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;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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}

Comment thread fuzz/CMakeLists.txt
Comment on lines +1 to +4
# libFuzzer-based fuzz targets for jsonnet.
# Only built with -DBUILD_FUZZERS=ON and requires clang. See README.md
# in this directory.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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 branch has not been deployed

No deployments
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.

1 participant