Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughLarge-scale modernization: migrate many public APIs to std::string_view/span, add RAII socket/address helpers, centralize config and string/JSON utilities, modernize network probes/scanners with stricter parsing and non-blocking TLS, and update build/test orchestration and tests. ChangesComprehensive modernization and build orchestration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/network/tcp_async_scan.cpp (1)
643-676:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftProcess-global abort state makes concurrent SYN scans race each other.
g_syn_abortand the temporary SIGINT/SIGTERM handler are shared across the whole process, so two overlapping SYN scans can clobber each other: the later guard resets the first scan's abort flag, a signal aborts both scans, and the first destructor can restore handlers while the second scan is still running. Please either serialize SYN scans globally or move to a single registered handler with per-scan registration/state management.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/network/tcp_async_scan.cpp` around lines 643 - 676, The code uses a process-global g_syn_abort and installs per-scan SIGINT/SIGTERM handlers in SynAbortSignalGuard, causing races between overlapping SYN scans; fix by removing per-scan signal handler installs and either (A) serialize scans so only one can run at a time (wrap scan entry/exit with a global mutex/lock and keep SynAbortSignalGuard to only manage g_syn_abort while ensuring no concurrent guards exist) or (B) implement a single global signal manager: register one process-wide handler (syn_abort_signal_handler) that notifies a central SynAbortManager which tracks per-scan tokens/flags and can mark the specific scan instances as aborted (replace g_syn_abort with per-scan atomic flags maintained by SynAbortManager, and change SynAbortSignalGuard to register/unregister its token with the manager instead of calling sigaction). Use the existing symbols g_syn_abort, syn_abort_signal_handler, and SynAbortSignalGuard as the reference points when applying the chosen approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/analysis/brand_cert.cpp`:
- Around line 175-185: asn_owns_brand currently compares brand_domain directly
against entries in kBrandTable and can miss matches for non-canonical inputs
like "*.Google.com" or "AWS.Amazon.com"; before the table lookup in
asn_owns_brand, normalize brand_domain the same way is_brand does (strip
wildcard prefixes, lower-case the host, and collapse known aliases) so the
find_if on kBrandTable will compare canonical forms; update asn_owns_brand to
transform the incoming brand_domain into its canonical form and then perform the
ranges::find_if against kBrandTable (referencing asn_owns_brand, kBrandTable,
and the normalization logic used by is_brand).
In `@src/analysis/geoip.cpp`:
- Around line 250-263: The function geo_sypex currently leaves GeoInfo::ip empty
when called with an empty input (used to discover the caller's IP), which makes
GeoInfo::ok() falsely fail; update geo_sypex to populate g.ip even when the
input ip is empty by first using the passed ip if non-empty, otherwise
extracting the resolved IP from the Sypex response (e.g., call
json_get_str(r.body, "ip") or the appropriate JSON key) and assign it to g.ip
before returning; keep existing error handling and other field assignments
(references: GeoInfo, geo_sypex, g.ip, r.body, json_get_str).
In `@src/core/utils.cpp`:
- Around line 253-287: The unescape loop currently drops handling for '\b' and
'\f' and falls through to default appending 'b'/'f'; update the switch inside
unescape() (the loop in parse_string()/unescape() that examines escaped char c
and calls parse_u16/append_utf8) to add case 'b': result += '\b'; break; and
case 'f': result += '\f'; break; so that backspace and formfeed escapes are
decoded correctly (leave existing cases for 'n','r','t','"','\\','u' and
surrogate logic unchanged).
In `@src/core/utils.h`:
- Around line 4-25: The modules branch is missing the C stdio import so the type
FILE used by config::g_save_fp is undefined; add import <cstdio>; to the list
inside the `#if defined(__cpp_modules) && __cpp_modules >= 201907L` branch
(alongside import <string>, import <vector>, etc.) so that FILE is available
when modules are enabled and compilation succeeds.
In `@src/network/dns.cpp`:
- Around line 95-106: The loop that builds v4_ips and v6_ips uses
extract_ip(p->ai_addr) which can return an empty string; before calling
std::ranges::find or pushing into v4_ips/v6_ips inside the for (const auto*
p{ai.get()}; p; p = p->ai_next) loop, check whether the extracted ip is empty
and skip (continue) if so; only perform deduplication and std::move(ip) into the
corresponding vector when ip.empty() is false to avoid inserting invalid empty
entries (refer to extract_ip, v4_ips, v6_ips and the loop body).
In `@src/network/http_client.cpp`:
- Around line 242-250: The host/path split logic currently finds '?' before
handling fragments, causing URLs like "http://example.com#frag?x" to treat the
'?' as a query and corrupt host parsing; update the parsing in http_client.cpp
(the code that computes slash_pos, query_pos, frag_pos and split_pos used for
host/path extraction) to either strip/truncate the input view at frag_pos before
searching for '?' and '/' or, if keeping all positions, ensure any query_pos is
ignored when it falls after frag_pos (i.e., only consider query_pos if frag_pos
== npos or query_pos < frag_pos); apply the same fix to the later analogous
block around the code handling lines 255-272 so fragment-contained '?' never
acts as a query separator for host/path splitting.
In `@src/network/service_probes.cpp`:
- Around line 52-53: The calls to tcp_send_all(s, req.data(),
static_cast<int>(req.size())) in service_probes.cpp are not checked, so partial
or failed sends fall through to recv and misclassify probes; update each call
site (the tcp_send_all invocation in the probe functions that use socket
variable s and request buffer req) to verify the return value equals the
requested byte count (or is non-negative/full-send per tcp_send_all's contract),
and on mismatch or error log/handle the error and skip calling recv (return or
mark probe as failed) to avoid waiting on a response for an incomplete send.
- Around line 16-26: The loop in printable_prefix may append two-character
escapes ("\\r" / "\\n") without ensuring there's room, potentially exceeding
lim; update the handling in the loop (the block iterating s and building out) so
that before appending a two-character escape you check if out.size() + 2 <= lim
and only then append "\\r" or "\\n"; otherwise if there's exactly one slot left
(out.size() < lim) append a single '.' (or another single-char placeholder) to
stay within lim; keep the existing single-char append behavior for printable
characters and '.' for other bytes and leave the loop termination condition
(out.size() < lim) as-is.
In `@src/network/tls_probe.cpp`:
- Around line 174-227: The code currently treats ssl_res == -1 as a timeout, but
SSL_connect can return -1 for many non-timeout errors; add a boolean flag (e.g.,
timed_out) that is set true only when you explicitly set ssl_res = -1 because
the deadline expired or select() returned <= 0, preserve existing ssl_res
semantics elsewhere, and after the handshake loop use that flag (not ssl_res ==
-1) to decide whether to set r.err = "timeout during tls handshake"; otherwise
call SSL_get_error(ssl.get(), ssl_res) and ERR_get_error()/ERR_error_string_n to
populate r.err with the real OpenSSL error and include the SSL error code for
diagnostics (refer to symbols SSL_connect, SSL_get_error, ssl_res, ssl_err,
ERR_get_error, ERR_error_string_n, and the handshake loop where select() and
deadline are checked).
- Around line 156-161: The ALPN token length is cast to unsigned char without
bounds checking in the loop that builds wire (see split(alpn, ','), trim(p), and
the push_back(static_cast<unsigned char>(v.size()))); add a guard that checks if
v.size() > 255 before encoding and handle it (e.g., log/error out or skip the
token) so the length byte cannot wrap; ensure you update the code path that
writes the length byte and subsequently calls std::ranges::transform to only run
for tokens with v.size() <= 255.
In `@src/network/tls_probe.h`:
- Around line 32-34: The expiring_soon() helper currently treats
default-initialized probes (days_left == 0) as "expiring soon", causing false
positives; update expiring_soon() to first ensure certificate data is present
(e.g., check an existing boolean like has_certificate or change the sentinel so
absent == -1) and only return true when certificate is present and days_left is
in the 0..6 window (for example: has_certificate && days_left >= 0 && days_left
< 7 or use days_left > 0 if you prefer excluding zero). Modify the struct/state
that holds days_left if needed to introduce or set a clear sentinel (e.g., -1)
on parse failure and reference expiring_soon and days_left in your fix so logic
only signals "expiring soon" when real certificate data exists.
In `@src/network/udp_scanner.cpp`:
- Around line 131-160: The code treats recv() == 0 as a timeout because
saved_err is set when got <= 0; change the logic to only read the socket error
when got < 0 (call WSAGetLastError() only for negative returns) and treat got >=
0 as a valid UDP response. Concretely, update the recv handling around the
::recv() call and the saved_err/werr variables so saved_err is 0 when got >= 0,
keep the non-Windows normalization of werr based on saved_err, and ensure the
branch that marks a response (setting result.responded, result.bytes, and
result.reply_hex) runs for got >= 0 (with bytes==0 and an empty/appropriate
reply_hex for zero-length datagrams) instead of treating got == 0 as a timeout
via is_timeout_error(werr).
In `@src/network/udp_scanner.h`:
- Around line 28-33: The udp_probe(raw-pointer overload) must validate inputs
before building the span: in udp_probe(const std::string& host, int port, const
unsigned char* payload, int plen, int timeout_ms) check that plen is
non-negative and that if plen > 0 then payload is not nullptr (plen == 0 may
allow a nullptr payload); on invalid input return an appropriate UdpResult error
(e.g. invalid-argument / failure) instead of creating std::span with a wrapped
size or null pointer, then forward the validated span to the existing
span-overload.
---
Outside diff comments:
In `@src/network/tcp_async_scan.cpp`:
- Around line 643-676: The code uses a process-global g_syn_abort and installs
per-scan SIGINT/SIGTERM handlers in SynAbortSignalGuard, causing races between
overlapping SYN scans; fix by removing per-scan signal handler installs and
either (A) serialize scans so only one can run at a time (wrap scan entry/exit
with a global mutex/lock and keep SynAbortSignalGuard to only manage g_syn_abort
while ensuring no concurrent guards exist) or (B) implement a single global
signal manager: register one process-wide handler (syn_abort_signal_handler)
that notifies a central SynAbortManager which tracks per-scan tokens/flags and
can mark the specific scan instances as aborted (replace g_syn_abort with
per-scan atomic flags maintained by SynAbortManager, and change
SynAbortSignalGuard to register/unregister its token with the manager instead of
calling sigaction). Use the existing symbols g_syn_abort,
syn_abort_signal_handler, and SynAbortSignalGuard as the reference points when
applying the chosen approach.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2b547d1b-8792-499a-bd65-5c067c0a0cb7
📒 Files selected for processing (47)
CMakeLists.txtlocal_build.shrun_single_test.shsrc/analysis/brand_cert.cppsrc/analysis/brand_cert.hsrc/analysis/ct_check.cppsrc/analysis/ct_check.hsrc/analysis/geoip.cppsrc/analysis/geoip.hsrc/analysis/sni_consistency.cppsrc/analysis/sni_consistency.hsrc/analysis/verdict.cppsrc/analysis/verdict.hsrc/cli/orchestrator.cppsrc/cli/orchestrator.hsrc/core/utils.cppsrc/core/utils.hsrc/main.cppsrc/network/dns.cppsrc/network/dns.hsrc/network/http_client.cppsrc/network/http_client.hsrc/network/https_probe.cppsrc/network/https_probe.hsrc/network/j3_probes.cppsrc/network/j3_probes.hsrc/network/openssl_runtime.cppsrc/network/openssl_runtime.hsrc/network/port_scan.cppsrc/network/port_scan.hsrc/network/service_probes.cppsrc/network/service_probes.hsrc/network/socket_sys.hsrc/network/tcp_async_scan.cppsrc/network/tcp_async_scan.hsrc/network/tcp_scanner.cppsrc/network/tcp_scanner.hsrc/network/tls_probe.cppsrc/network/tls_probe.hsrc/network/udp_scanner.cppsrc/network/udp_scanner.hsrc/network/vpn_probes.cppsrc/network/vpn_probes.htests/CMakeLists.txttests/test_ct_check.cpptests/test_port_scan.cpptests/test_tcp_scanner.cpp
💤 Files with no reviewable changes (1)
- run_single_test.sh
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/network/tls_probe.cpp (1)
207-228:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't treat
select()errors as handshake timeouts.Line 208 still sets
timed_out = truefor bothsr == 0andsr == -1.select()returning-1is an error path, not a timeout, so this can still collapse real socket/OpenSSL failures into"timeout during tls handshake"at Line 220 and hide the actual diagnostic.Suggested fix
- if (sr <= 0) { - timed_out = true; - break; - } + if (sr == 0) { + timed_out = true; + break; + } + if (sr < 0) { + break; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/network/tls_probe.cpp` around lines 207 - 228, The code incorrectly treats select() errors (sr == -1) as timeouts by setting timed_out = true; change the select() handling so only sr == 0 sets timed_out = true, while sr < 0 is treated as an error: record/format errno (e.g. via strerror(errno)) into r.err or a new error message, clear timed_out, and break; keep the rest of the TLS handshake logic that checks ssl_res/SSL_get_error intact so actual socket/OpenSSL errors are reported rather than collapsed into "timeout during tls handshake". Ensure you update the branch around sr/timed_out (references: sr, timed_out, select(), ssl_res, r.err, SSL_get_error) accordingly.src/analysis/brand_cert.cpp (1)
181-193:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCanonicalize
brand_domainviais_brand()before the table lookup.This still misses valid inputs like
www.google.com: lowercasing plus stripping*.is not enough, becauseasn_owns_brand()only does an exact match whilecert_claims_brand()accepts suffix matches. Reuseis_brand()here so both paths resolve the same canonical brand.Proposed fix
- std::string ln{brand_domain}; - std::ranges::transform(ln, ln.begin(), [](unsigned char c) { - return static_cast<char>(std::tolower(c)); - }); - - if (ln.size() > 2 && ln[0] == '*' && ln[1] == '.') { - ln = ln.substr(2); - } + const char* normalized{is_brand(brand_domain)}; + if (!normalized) return false; // Find markers for brand const auto it = std::ranges::find_if(kBrandTable, [&](const auto& entry) { - return ln == entry.brand; + return std::string_view{entry.brand} == normalized; });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/analysis/brand_cert.cpp` around lines 181 - 193, The lookup compares a lowercased/stripped brand_domain (ln) only for exact matches, missing cases like "www.google.com"; replace the ad-hoc normalization with a call to is_brand(brand_domain) to produce the canonical brand string before searching kBrandTable so asn_owns_brand() and cert_claims_brand() use the same canonical form. In practice: compute a canonical string by calling is_brand(brand_domain) (or the function that returns the canonical brand), lowercase/normalize that result if needed, then use that canonical value in the std::ranges::find_if against kBrandTable (referencing ln/brand_domain, is_brand(), and kBrandTable) so suffix and wildcard cases resolve consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@local_build.py`:
- Around line 732-746: The current code uses a predictable shared path
st.log_path = f"/tmp/bbvpn_stage_{st.key}.log" which is vulnerable to races and
symlink attacks; replace this by creating a secure per-run temp file (use
tempfile.mkstemp() or tempfile.NamedTemporaryFile(delete=False)), set
st.log_path to that generated filename, open the returned file descriptor/path
for writing and pass the safe file object to await st.func(st, log_file), and
ensure you close the fd/file and remove the temp file when done; adjust the
spinner_task/spin_task usage around the new temp-file creation so the log file
is created before st.func is invoked.
In `@local_build.sh`:
- Around line 37-64: Replace the predictable STATE_FILE and stage-log creation
with secure mktemp usage: instead of STATE_FILE="/tmp/bbvpn_state_$$.sh" create
the state file atomically with something like STATE_FILE="$(mktemp
/tmp/bbvpn_state.XXXXXX.sh)" (or mktemp --tmpdir with a template), set
restrictive permissions (chmod 600 "$STATE_FILE"), and only then write/redirect
into and source it; do the same for per-key stage logs by creating them via
mktemp with a template that includes the key (e.g. mktemp
"/tmp/bbvpn_stage_${key}.XXXXXX.log"), ensure umask is tight and files are
created before being used, and update cleanup() to remove those mktemp-created
files—this prevents predictable filenames and symlink/clobber races when
sourcing or writing logs.
---
Duplicate comments:
In `@src/analysis/brand_cert.cpp`:
- Around line 181-193: The lookup compares a lowercased/stripped brand_domain
(ln) only for exact matches, missing cases like "www.google.com"; replace the
ad-hoc normalization with a call to is_brand(brand_domain) to produce the
canonical brand string before searching kBrandTable so asn_owns_brand() and
cert_claims_brand() use the same canonical form. In practice: compute a
canonical string by calling is_brand(brand_domain) (or the function that returns
the canonical brand), lowercase/normalize that result if needed, then use that
canonical value in the std::ranges::find_if against kBrandTable (referencing
ln/brand_domain, is_brand(), and kBrandTable) so suffix and wildcard cases
resolve consistently.
In `@src/network/tls_probe.cpp`:
- Around line 207-228: The code incorrectly treats select() errors (sr == -1) as
timeouts by setting timed_out = true; change the select() handling so only sr ==
0 sets timed_out = true, while sr < 0 is treated as an error: record/format
errno (e.g. via strerror(errno)) into r.err or a new error message, clear
timed_out, and break; keep the rest of the TLS handshake logic that checks
ssl_res/SSL_get_error intact so actual socket/OpenSSL errors are reported rather
than collapsed into "timeout during tls handshake". Ensure you update the branch
around sr/timed_out (references: sr, timed_out, select(), ssl_res, r.err,
SSL_get_error) accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 95db7418-ef8a-461f-8f0f-123e850141b3
📒 Files selected for processing (18)
.github/workflows/debug.yml.github/workflows/release.ymllocal_build.pylocal_build.shsrc/analysis/brand_cert.cppsrc/analysis/geoip.cppsrc/core/utils.cppsrc/core/utils.hsrc/network/dns.cppsrc/network/http_client.cppsrc/network/service_probes.cppsrc/network/tcp_async_scan.cppsrc/network/tls_probe.cppsrc/network/tls_probe.hsrc/network/udp_scanner.cppsrc/network/udp_scanner.htests/test_http_client.cpptests/test_utils.cpp
🚧 Files skipped from review as they are similar to previous changes (8)
- src/network/udp_scanner.h
- src/network/dns.cpp
- src/network/tcp_async_scan.cpp
- src/network/udp_scanner.cpp
- src/network/service_probes.cpp
- src/core/utils.h
- src/core/utils.cpp
- src/network/http_client.cpp
Owner
Author
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/network/openssl_runtime.cpp (1)
80-86:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't tear down the OpenSSL 3 runtime behind
std::call_once.After
openssl_runtime_cleanup()unloads the providers,openssl_runtime_init()can never rebuild them becauseg_ossl_onceis already spent. The next init call will keep returning the oldg_ossl_okstate against a torn-down runtime. Either make cleanup a no-op for the OpenSSL 3 default context, or replace the once-flag with resettable lifecycle state.Possible minimal fix
void openssl_runtime_cleanup() { `#if` OPENSSL_VERSION_NUMBER >= 0x30000000L - if (g_provider_legacy) { - OSSL_PROVIDER_unload(g_provider_legacy); - g_provider_legacy = nullptr; - } - if (g_provider_base) { - OSSL_PROVIDER_unload(g_provider_base); - g_provider_base = nullptr; - } - if (g_provider_default) { - OSSL_PROVIDER_unload(g_provider_default); - g_provider_default = nullptr; - } + // Providers are loaded into the process-global default library context. + // Keep them alive for the lifetime of the process so call_once-based init + // cannot report success after teardown. + return; `#else` EVP_cleanup(); CRYPTO_cleanup_all_ex_data(); ERR_free_strings(); `#endif`Also applies to: 89-108
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/network/openssl_runtime.cpp` around lines 80 - 86, The init uses std::call_once (g_ossl_once) so once cleanup runs the runtime cannot be reinitialized; replace call_once with an explicit mutex + state so init can be repeated after cleanup. Concretely, in openssl_runtime_init() remove std::call_once(g_ossl_once, do_init) and instead lock a mutex (e.g., g_ossl_mutex), check a boolean state (e.g., g_ossl_initialized), and if false call do_init(), set g_ossl_initialized and g_ossl_ok/g_ossl_err accordingly, then unlock and return g_ossl_ok; in openssl_runtime_cleanup() acquire the same mutex and properly unload providers and then reset g_ossl_initialized = false and clear/adjust g_ossl_ok and g_ossl_err so subsequent openssl_runtime_init() can rebuild the runtime; reference symbols: openssl_runtime_init, openssl_runtime_cleanup, g_ossl_once, do_init, g_ossl_ok, g_ossl_err..github/workflows/debug.yml (1)
30-34:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCI: install the Debian package that provides
run-clang-tidy
In.github/workflows/debug.yml(around lines 30-34), the job installsclang-tidybut then invokesrun-clang-tidy; Debian’sclang-tidypackage in trixie does not ship therun-clang-tidyexecutable (the wrapper is provided byclang-tools/clang-tools-<version>). This can fail withcommand not foundbefore checks run.
- Fix: add
clang-tools(or the matchingclang-tools-<version>for the installed clang/clang-tidy version) to theapt-get installlist, or switch to the exact script/binary that’s present in the installed package.
Also applies to the similar block around 68-70.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/debug.yml around lines 30 - 34, The CI installs clang-tidy but later invokes run-clang-tidy which is not provided by the plain clang-tidy package; update the "Install dependencies" apt-get install line that currently lists clang-tidy to also install clang-tools or the matching clang-tools-<version> package so run-clang-tidy exists (do the same change for the similar install block around the other job). Locate the "Install dependencies" step(s) and add clang-tools or clang-tools-<version> to the package list so the run-clang-tidy wrapper is available at runtime.src/core/utils.cpp (1)
272-285:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace lone UTF-16 surrogates instead of emitting invalid UTF-8.
If
\uD800-\uDFFFappears without a valid pair, the fallback path still callsappend_utf8(u1). That encodes an illegal Unicode scalar and can leak invalid UTF-8 into parsed JSON strings.Proposed fix
case 'u': { const int u1 = parse_u16(i + 1); if (u1 < 0) { result += '?'; break; } i += 4; // Check for UTF-16 surrogate pair - if (u1 >= 0xD800 && u1 <= 0xDBFF && - i + 2 < s.size() && s.at(i + 1) == '\\' && s.at(i + 2) == 'u') { - const int u2 = parse_u16(i + 3); - if (u2 >= 0xDC00 && u2 <= 0xDFFF) { - const uint32_t cp = 0x10000 + - ((static_cast<uint32_t>(u1 - 0xD800) << 10) | - static_cast<uint32_t>(u2 - 0xDC00)); - append_utf8(cp); - i += 6; // skip \uXXXX of the low surrogate - break; - } + if (u1 >= 0xD800 && u1 <= 0xDBFF) { + if (i + 2 < s.size() && s.at(i + 1) == '\\' && s.at(i + 2) == 'u') { + const int u2 = parse_u16(i + 3); + if (u2 >= 0xDC00 && u2 <= 0xDFFF) { + const uint32_t cp = 0x10000 + + ((static_cast<uint32_t>(u1 - 0xD800) << 10) | + static_cast<uint32_t>(u2 - 0xDC00)); + append_utf8(cp); + i += 6; // skip \uXXXX of the low surrogate + break; + } + } + result += "\xEF\xBF\xBD"; + break; + } + if (u1 >= 0xDC00 && u1 <= 0xDFFF) { + result += "\xEF\xBF\xBD"; + break; } append_utf8(static_cast<uint32_t>(u1)); break; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/utils.cpp` around lines 272 - 285, The code currently emits an invalid Unicode scalar by calling append_utf8(static_cast<uint32_t>(u1)) when a UTF-16 high/low surrogate pair is not valid; instead detect the lone surrogate (u1 in 0xD800–0xDFFF) and append the Unicode replacement character U+FFFD (0xFFFD) via append_utf8(0xFFFD) rather than emitting u1 (preserve existing control flow and breaks), keeping parse_u16, append_utf8, u1 and u2 as the locating symbols to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@local_build.sh`:
- Around line 37-38: The state file is being written with raw quoted values
which will be executed when sourced (STATE_FILE), so create and use a single
helper (e.g., append_escaped_state or write_state) that shell-escapes scalar
values before appending lines to STATE_FILE and replace all raw echo appends
(for VCPKG_ROOT, VCPKG_TOOLCHAIN, OVERLAY_TRIPLETS, TEST_CMD, etc.) to call that
helper; implement the helper to produce safe shell-assignments (use printf '%q'
or equivalent to escape the value and then append a line like
VAR=<escaped-value> to "$STATE_FILE") so any embedded $(), backticks or quotes
cannot be executed when sourcing.
In `@src/main.cpp`:
- Line 296: The menu label is rendering the literal ".at(0)" because the
tee_printf format string contains that text instead of injecting the exit key;
update the tee_printf call so it prints the actual exit key character (replace
the format string " %s.at(0)%s Exit" with one that inserts the key, e.g. " %c
Exit" and pass exit_key.at(0) or exit_key[0], or use "%s" with exit_key.c_str()
if you want the whole string); change the tee_printf invocation (the call to
tee_printf shown) to use the actual exit key variable (exit_key or exitKey)
rather than the literal ".at(0)".
In `@src/network/tcp_async_scan.cpp`:
- Around line 676-680: The current code installs SIGINT and SIGTERM handlers
with a single `&&` so if sigaction(SIGINT, &new_action, &tuple.old_int) succeeds
but sigaction(SIGTERM, &new_action, &tuple.old_term) fails the new SIGINT
handler remains installed; update the logic in the block around SigactionTuple,
sigaction, SIGINT, SIGTERM, new_action, tuple.old_int, tuple.old_term and
guard_.reset to perform the second sigaction separately and on failure undo the
first installation by restoring the old SIGINT handler (using sigaction with
tuple.old_int) before returning/propagating the error, only calling
guard_.reset(tuple) when both installations succeed.
---
Outside diff comments:
In @.github/workflows/debug.yml:
- Around line 30-34: The CI installs clang-tidy but later invokes run-clang-tidy
which is not provided by the plain clang-tidy package; update the "Install
dependencies" apt-get install line that currently lists clang-tidy to also
install clang-tools or the matching clang-tools-<version> package so
run-clang-tidy exists (do the same change for the similar install block around
the other job). Locate the "Install dependencies" step(s) and add clang-tools or
clang-tools-<version> to the package list so the run-clang-tidy wrapper is
available at runtime.
In `@src/core/utils.cpp`:
- Around line 272-285: The code currently emits an invalid Unicode scalar by
calling append_utf8(static_cast<uint32_t>(u1)) when a UTF-16 high/low surrogate
pair is not valid; instead detect the lone surrogate (u1 in 0xD800–0xDFFF) and
append the Unicode replacement character U+FFFD (0xFFFD) via append_utf8(0xFFFD)
rather than emitting u1 (preserve existing control flow and breaks), keeping
parse_u16, append_utf8, u1 and u2 as the locating symbols to change.
In `@src/network/openssl_runtime.cpp`:
- Around line 80-86: The init uses std::call_once (g_ossl_once) so once cleanup
runs the runtime cannot be reinitialized; replace call_once with an explicit
mutex + state so init can be repeated after cleanup. Concretely, in
openssl_runtime_init() remove std::call_once(g_ossl_once, do_init) and instead
lock a mutex (e.g., g_ossl_mutex), check a boolean state (e.g.,
g_ossl_initialized), and if false call do_init(), set g_ossl_initialized and
g_ossl_ok/g_ossl_err accordingly, then unlock and return g_ossl_ok; in
openssl_runtime_cleanup() acquire the same mutex and properly unload providers
and then reset g_ossl_initialized = false and clear/adjust g_ossl_ok and
g_ossl_err so subsequent openssl_runtime_init() can rebuild the runtime;
reference symbols: openssl_runtime_init, openssl_runtime_cleanup, g_ossl_once,
do_init, g_ossl_ok, g_ossl_err.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 786e2aa1-1a3b-4904-b65d-d1173c7b8c18
📒 Files selected for processing (21)
.clang-tidy.github/workflows/debug.ymlCMakeLists.txtlocal_build.pylocal_build.shsrc/analysis/brand_cert.cppsrc/analysis/ct_check.cppsrc/analysis/sni_consistency.cppsrc/cli/orchestrator.cppsrc/core/utils.cppsrc/main.cppsrc/network/http_client.cppsrc/network/https_probe.cppsrc/network/j3_probes.cppsrc/network/openssl_runtime.cppsrc/network/port_scan.cppsrc/network/service_probes.cppsrc/network/socket_sys.hsrc/network/tcp_async_scan.cppsrc/network/tls_probe.cppsrc/network/vpn_probes.cpp
🚧 Files skipped from review as they are similar to previous changes (12)
- src/network/j3_probes.cpp
- src/network/port_scan.cpp
- src/analysis/sni_consistency.cpp
- src/network/vpn_probes.cpp
- src/network/tls_probe.cpp
- src/cli/orchestrator.cpp
- src/network/service_probes.cpp
- src/analysis/brand_cert.cpp
- local_build.py
- src/network/http_client.cpp
- src/network/https_probe.cpp
- src/analysis/ct_check.cpp
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Chores
Refactor
User-facing
Tests