Copilot commented on code in PR #3676:
URL: https://github.com/apache/brpc/pull/3676#discussion_r4388285508
##########
test/CMakeLists.txt:
##########
@@ -345,7 +345,7 @@ if(BUILD_FUZZ_TESTS)
fuzz_hulu fuzz_json fuzz_redis fuzz_shead fuzz_sofa fuzz_uri
fuzz_baidu_rpc fuzz_mongo fuzz_memcache
fuzz_couchbase fuzz_streaming fuzz_http_parser fuzz_amf
- fuzz_rtmp)
+ fuzz_rtmp fuzz_thrift)
Review Comment:
With `BUILD_FUZZ_TESTS=ON` and the default `WITH_THRIFT=OFF`, this target
calls `ParseThriftMessage`, but CMake excludes the Thrift implementation
sources, causing an undefined-reference link failure. Append `fuzz_thrift` only
when `WITH_THRIFT` is enabled; enabling it in the OSS-Fuzz script does not
protect other CMake builds.
##########
test/fuzzing/fuzz_thrift_seed_corpus/short.bin:
##########
@@ -0,0 +1 @@
+\x00\x00\x00
Review Comment:
This seed contains literal ASCII escape sequences rather than NUL bytes, so
it reaches the invalid-version branch instead of exercising a truncated header.
Regenerate it as 5–7 raw binary bytes: the harness rejects inputs shorter than
five bytes, while the parser requires eight bytes for its header.
##########
test/fuzzing/oss-fuzz.sh:
##########
@@ -29,21 +29,21 @@ cmake \
-DCMAKE_C_COMPILER="$CC" -DCMAKE_CXX_COMPILER="$CXX" \
-DCMAKE_C_FLAGS="$CFLAGS" -DCMAKE_CXX_FLAGS="$CFLAGS"
-DCMAKE_CPP_FLAGS="$CFLAGS" \
-DCMAKE_EXE_LINKER_FLAGS="$CFLAGS"
-DLIB_FUZZING_ENGINE="$LIB_FUZZING_ENGINE" \
- -DCMAKE_BUILD_TYPE=Debug -DBUILD_SHARED_LIBS=OFF -DWITH_SNAPPY=ON
-DBUILD_UNIT_TESTS=ON -DBUILD_FUZZ_TESTS=ON ../.
+ -DCMAKE_BUILD_TYPE=Debug -DBUILD_SHARED_LIBS=OFF -DWITH_SNAPPY=ON
-DBUILD_UNIT_TESTS=ON -DBUILD_FUZZ_TESTS=ON -DWITH_THRIFT=ON ../.
# https://github.com/google/oss-fuzz/pull/10898
make \
fuzz_butil fuzz_esp fuzz_hpack fuzz_http fuzz_hulu fuzz_json \
fuzz_redis fuzz_shead fuzz_sofa fuzz_uri \
fuzz_baidu_rpc fuzz_mongo fuzz_memcache \
fuzz_couchbase fuzz_streaming fuzz_http_parser \
- fuzz_amf fuzz_rtmp --ignore-errors -j$(nproc)
+ fuzz_amf fuzz_rtmp fuzz_thrift --ignore-errors -j$(nproc)
Review Comment:
The newly enabled harness discards the result of `ParseThriftMessage` in
`test/fuzzing/fuzz_thrift.cpp:34`, leaking a `MostCommonMessage` and its
payload on every successful parse. Repeated valid inputs can exhaust the
fuzzer's memory. Capture the result and call `result.message()->Destroy()` when
`result.is_ok()` is true before enabling this target.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
