Copilot commented on code in PR #3679: URL: https://github.com/apache/brpc/pull/3679#discussion_r4548829541
########## test/fuzzing/fuzz_concurrency.cpp: ########## @@ -0,0 +1,65 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to You under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and +// limitations under the License. + +// Fuzz the auto concurrency limiter (src/brpc/policy/auto_concurrency_limiter.cpp): +// fuzz bytes drive a stream of (error_code, latency_us) responses and +// request ticks through the real gradient-based limiter, including the +// recovery/reset paths exercised by bursts of errors. +// +// OnResponded() stamps samples with the real clock: filling a sample window +// needs >=100us of wall time per sample and remeasurement cannot start +// until ~25s after construction, so coverage would depend on execution +// speed rather than on the input bytes. Samples are therefore fed to +// AddSample() at a synthetic clock that every 8-byte record advances -- +// the same sampling approach as test/brpc_auto_concurrency_limiter_unittest.cpp +// -- which lets an input jump straight across the sample window and the +// remeasure/reset deadlines. + +#include "butil/time.h" +#include "brpc/policy/auto_concurrency_limiter.h" + +extern "C" int +LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) Review Comment: `uint8_t` and `size_t` (and later `uint64_t`/`int64_t`) are used without including standard headers that define them. This can fail to compile depending on transitive includes. Add the appropriate standard includes (e.g., `<cstdint>` and `<cstddef>` or equivalents) directly in this file to make the harness self-contained. ########## test/fuzzing/fuzz_concurrency.cpp: ########## @@ -0,0 +1,65 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to You under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and +// limitations under the License. + +// Fuzz the auto concurrency limiter (src/brpc/policy/auto_concurrency_limiter.cpp): +// fuzz bytes drive a stream of (error_code, latency_us) responses and +// request ticks through the real gradient-based limiter, including the +// recovery/reset paths exercised by bursts of errors. +// +// OnResponded() stamps samples with the real clock: filling a sample window +// needs >=100us of wall time per sample and remeasurement cannot start +// until ~25s after construction, so coverage would depend on execution +// speed rather than on the input bytes. Samples are therefore fed to +// AddSample() at a synthetic clock that every 8-byte record advances -- +// the same sampling approach as test/brpc_auto_concurrency_limiter_unittest.cpp +// -- which lets an input jump straight across the sample window and the +// remeasure/reset deadlines. + +#include "butil/time.h" +#include "brpc/policy/auto_concurrency_limiter.h" + +extern "C" int +LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) +{ + if (size < 8) { + return 0; + } + + brpc::policy::AutoConcurrencyLimiter limiter; + int64_t now_us = butil::cpuwide_time_us(); + + const size_t n = size / 8; + for (size_t i = 0; i < n; ++i) { + uint64_t v = 0; + for (size_t b = 0; b < 8; ++b) { + v = (v << 8) | data[i * 8 + b]; + } + // Advance the synthetic clock by 1us..~16.8s per record. The +1us + // floor keeps the sampled window width strictly positive -- + // UpdateMaxConcurrency() divides by it when computing qps. + now_us += static_cast<int64_t>((v >> 40) + 1); + const int error_code = static_cast<int>(v & 0xff); + const int64_t latency_us = static_cast<int64_t>((v >> 8) & 0xffffff); + limiter.AddSample(error_code, latency_us, now_us); + limiter.MaxConcurrency(); + if ((v & 0x100) || (i & 7) == 0) { + // a request tick (current concurrency taken from the stream) + limiter.OnRequested(static_cast<int>((v >> 20) % 5096), nullptr); Review Comment: Passing `nullptr` into `OnRequested` risks turning this into a trivial/null-deref fuzzer (depending on whether the limiter dereferences the pointer). If the API expects a non-null context/controller, construct a minimal valid object (or use an overload that doesn’t require the pointer) so the fuzzer spends time exploring limiter state transitions rather than immediately crashing. ########## test/fuzzing/fuzz_concurrency.cpp: ########## @@ -0,0 +1,65 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to You under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and +// limitations under the License. + +// Fuzz the auto concurrency limiter (src/brpc/policy/auto_concurrency_limiter.cpp): +// fuzz bytes drive a stream of (error_code, latency_us) responses and +// request ticks through the real gradient-based limiter, including the +// recovery/reset paths exercised by bursts of errors. +// +// OnResponded() stamps samples with the real clock: filling a sample window +// needs >=100us of wall time per sample and remeasurement cannot start +// until ~25s after construction, so coverage would depend on execution +// speed rather than on the input bytes. Samples are therefore fed to +// AddSample() at a synthetic clock that every 8-byte record advances -- +// the same sampling approach as test/brpc_auto_concurrency_limiter_unittest.cpp +// -- which lets an input jump straight across the sample window and the +// remeasure/reset deadlines. + +#include "butil/time.h" +#include "brpc/policy/auto_concurrency_limiter.h" + +extern "C" int +LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) +{ + if (size < 8) { + return 0; + } + + brpc::policy::AutoConcurrencyLimiter limiter; + int64_t now_us = butil::cpuwide_time_us(); + + const size_t n = size / 8; + for (size_t i = 0; i < n; ++i) { + uint64_t v = 0; + for (size_t b = 0; b < 8; ++b) { + v = (v << 8) | data[i * 8 + b]; + } + // Advance the synthetic clock by 1us..~16.8s per record. The +1us + // floor keeps the sampled window width strictly positive -- + // UpdateMaxConcurrency() divides by it when computing qps. + now_us += static_cast<int64_t>((v >> 40) + 1); + const int error_code = static_cast<int>(v & 0xff); + const int64_t latency_us = static_cast<int64_t>((v >> 8) & 0xffffff); + limiter.AddSample(error_code, latency_us, now_us); + limiter.MaxConcurrency(); + if ((v & 0x100) || (i & 7) == 0) { + // a request tick (current concurrency taken from the stream) + limiter.OnRequested(static_cast<int>((v >> 20) % 5096), nullptr); Review Comment: This harness relies on several embedded protocol/magic constants (record size `8`, shifts/masks like `>> 40`, `0xffffff`, and the modulus `5096`) that define the input encoding and limiting range. Please factor these into named `constexpr` constants (and ideally add brief comments for `5096` and the bit layout), so future changes to the limiter or encoding don’t require re-deriving the bit math from scratch. ########## test/fuzzing/fuzz_concurrency.cpp: ########## @@ -0,0 +1,65 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to You under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and +// limitations under the License. + +// Fuzz the auto concurrency limiter (src/brpc/policy/auto_concurrency_limiter.cpp): +// fuzz bytes drive a stream of (error_code, latency_us) responses and +// request ticks through the real gradient-based limiter, including the +// recovery/reset paths exercised by bursts of errors. +// +// OnResponded() stamps samples with the real clock: filling a sample window +// needs >=100us of wall time per sample and remeasurement cannot start +// until ~25s after construction, so coverage would depend on execution +// speed rather than on the input bytes. Samples are therefore fed to +// AddSample() at a synthetic clock that every 8-byte record advances -- +// the same sampling approach as test/brpc_auto_concurrency_limiter_unittest.cpp +// -- which lets an input jump straight across the sample window and the +// remeasure/reset deadlines. + +#include "butil/time.h" +#include "brpc/policy/auto_concurrency_limiter.h" + +extern "C" int +LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) +{ + if (size < 8) { + return 0; + } + + brpc::policy::AutoConcurrencyLimiter limiter; + int64_t now_us = butil::cpuwide_time_us(); + + const size_t n = size / 8; + for (size_t i = 0; i < n; ++i) { + uint64_t v = 0; + for (size_t b = 0; b < 8; ++b) { + v = (v << 8) | data[i * 8 + b]; + } + // Advance the synthetic clock by 1us..~16.8s per record. The +1us + // floor keeps the sampled window width strictly positive -- + // UpdateMaxConcurrency() divides by it when computing qps. + now_us += static_cast<int64_t>((v >> 40) + 1); + const int error_code = static_cast<int>(v & 0xff); + const int64_t latency_us = static_cast<int64_t>((v >> 8) & 0xffffff); + limiter.AddSample(error_code, latency_us, now_us); + limiter.MaxConcurrency(); + if ((v & 0x100) || (i & 7) == 0) { + // a request tick (current concurrency taken from the stream) + limiter.OnRequested(static_cast<int>((v >> 20) % 5096), nullptr); Review Comment: This harness relies on several embedded protocol/magic constants (record size `8`, shifts/masks like `>> 40`, `0xffffff`, and the modulus `5096`) that define the input encoding and limiting range. Please factor these into named `constexpr` constants (and ideally add brief comments for `5096` and the bit layout), so future changes to the limiter or encoding don’t require re-deriving the bit math from scratch. ########## test/fuzzing/fuzz_concurrency.cpp: ########## @@ -0,0 +1,65 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to You under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and +// limitations under the License. + +// Fuzz the auto concurrency limiter (src/brpc/policy/auto_concurrency_limiter.cpp): +// fuzz bytes drive a stream of (error_code, latency_us) responses and +// request ticks through the real gradient-based limiter, including the +// recovery/reset paths exercised by bursts of errors. +// +// OnResponded() stamps samples with the real clock: filling a sample window +// needs >=100us of wall time per sample and remeasurement cannot start +// until ~25s after construction, so coverage would depend on execution +// speed rather than on the input bytes. Samples are therefore fed to +// AddSample() at a synthetic clock that every 8-byte record advances -- +// the same sampling approach as test/brpc_auto_concurrency_limiter_unittest.cpp +// -- which lets an input jump straight across the sample window and the +// remeasure/reset deadlines. + +#include "butil/time.h" +#include "brpc/policy/auto_concurrency_limiter.h" + +extern "C" int +LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) +{ + if (size < 8) { + return 0; + } + + brpc::policy::AutoConcurrencyLimiter limiter; + int64_t now_us = butil::cpuwide_time_us(); + + const size_t n = size / 8; + for (size_t i = 0; i < n; ++i) { + uint64_t v = 0; + for (size_t b = 0; b < 8; ++b) { + v = (v << 8) | data[i * 8 + b]; + } + // Advance the synthetic clock by 1us..~16.8s per record. The +1us + // floor keeps the sampled window width strictly positive -- + // UpdateMaxConcurrency() divides by it when computing qps. + now_us += static_cast<int64_t>((v >> 40) + 1); + const int error_code = static_cast<int>(v & 0xff); + const int64_t latency_us = static_cast<int64_t>((v >> 8) & 0xffffff); + limiter.AddSample(error_code, latency_us, now_us); + limiter.MaxConcurrency(); + if ((v & 0x100) || (i & 7) == 0) { + // a request tick (current concurrency taken from the stream) + limiter.OnRequested(static_cast<int>((v >> 20) % 5096), nullptr); Review Comment: This harness relies on several embedded protocol/magic constants (record size `8`, shifts/masks like `>> 40`, `0xffffff`, and the modulus `5096`) that define the input encoding and limiting range. Please factor these into named `constexpr` constants (and ideally add brief comments for `5096` and the bit layout), so future changes to the limiter or encoding don’t require re-deriving the bit math from scratch. -- 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]
