add sunnyquic inbound listener#1473
Open
happytrudy wants to merge 5 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds feature-gated SunnyQUIC inbound support with YAML configuration, TLS/QUIC options, authentication, TCP dispatch, validation, runtime user updates, and ShadowQUIC revision updates. ChangesSunnyQUIC inbound integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Config
participant InboundManager
participant SunnyQuicInbound
participant Dispatcher
Config->>InboundManager: Parse SunnyQUIC listener
InboundManager->>SunnyQuicInbound: Build handler with options and users
SunnyQuicInbound->>Dispatcher: Dispatch accepted TCP request
InboundManager->>SunnyQuicInbound: Send updated user list
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 (1)
clash-lib/src/app/inbound/manager.rs (1)
219-260: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGrowing per-protocol duplication for watch-channel creation/propagation.
Adding
SunnyQuiccontinues a pattern that is now duplicated 4 times in this file (provider hot-updateif letblocks at lines 219-260, and the(users_rx, users_tx)match at both theshadowsocks/not(shadowsocks)branches inload_inbound_providersat 298-333, and again instart_all_listenersat 401-429). Each new user-list-capable protocol requires touching 4 near-identical blocks across 2 functions. Consider extracting a shared helper, e.g.fn make_users_channel(opts: &InboundOpts) -> (Option<watch::Receiver<Vec<InboundUser>>>, Option<watch::Sender<Vec<InboundUser>>>)and a small helper for the hot-updateif letdispatch, to keep future protocol additions to a single call site.Also applies to: 298-333, 401-429
🤖 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 `@clash-lib/src/app/inbound/manager.rs` around lines 219 - 260, Refactor the duplicated per-protocol users watch-channel creation and hot-update dispatch around load_inbound_providers, start_all_listeners, and the provider update block. Introduce shared helpers such as make_users_channel and a users-update dispatch helper that encapsulate protocol-specific matching, then replace all four repeated blocks with those helpers while preserving feature gating and existing update behavior.
🤖 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 `@Cargo.toml`:
- Around line 48-52: Update the [patch.crates-io] entries for shadowquic and
shadowquic-macros to use their remote git repositories with pinned rev values
instead of ../shadowquic path dependencies. Keep local path overrides out of the
committed Cargo.toml while preserving the existing rustls and tokio-rustls
patches.
In `@clash-lib/src/config/internal/config.rs`:
- Around line 264-321: Strengthen parses_sunnyquic_example_file by matching
InboundOpts::SunnyQuic and asserting representative non-default fields from
sunnyquic-inbound.yaml, including congestion_control, initial_mtu, min_mtu,
max_path_num, gso, and mtu_discovery. Keep the existing parse and convert
checks, but verify these mapped values rather than only confirming the variant
exists.
In `@clash-lib/src/proxy/sunnyquic/mod.rs`:
- Around line 129-136: Run cargo +nightly fmt --all and apply the rustfmt
changes to the warn! invocations in the sunnyquic inbound request filtering
logic, including both referenced ranges. Commit the formatter-only output
without changing behavior.
---
Outside diff comments:
In `@clash-lib/src/app/inbound/manager.rs`:
- Around line 219-260: Refactor the duplicated per-protocol users watch-channel
creation and hot-update dispatch around load_inbound_providers,
start_all_listeners, and the provider update block. Introduce shared helpers
such as make_users_channel and a users-update dispatch helper that encapsulate
protocol-specific matching, then replace all four repeated blocks with those
helpers while preserving feature gating and existing update behavior.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fe8bb2d5-512a-4c2f-af5d-0a10d1787ada
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Cargo.tomlclash-bin/tests/data/config/full.yamlclash-lib/Cargo.tomlclash-lib/src/app/inbound/manager.rsclash-lib/src/app/inbound/network_listener.rsclash-lib/src/config/internal/config.rsclash-lib/src/config/internal/listener.rsclash-lib/src/proxy/mod.rsclash-lib/src/proxy/sunnyquic/mod.rsclash-lib/src/session.rssunnyquic-inbound.yaml
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 67
🤖 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 `@clash-lib/src/proxy/sunnyquic/mod.rs`:
- Around line 125-130: Update the Event::Request handling around
dispatch_request to obtain the QUIC connection’s remote address, thread it
through ProxyRequest, and pass that address as the session source instead of
unspecified_source(self.addr). Preserve the real client address for downstream
Session::source observability and policy.
In `@vendor/shadowquic/Cargo.toml`:
- Around line 1-20: Update the workspace configuration in Cargo.toml by adding a
release profile with size-oriented optimization, link-time optimization enabled,
and symbol stripping configured, while leaving the existing workspace package
and dependency settings unchanged.
In `@vendor/shadowquic/shadowquic-macros/src/lib.rs`:
- Around line 207-209: Update the size-tagged payload encode/decode paths around
len.encode and buffer allocation to enforce the protocol’s maximum payload size
before framing or allocating. Convert buffer lengths with u32::try_from instead
of truncating casts, reject oversized payloads, and ensure both encode locations
and the corresponding decode path preserve frame synchronization.
In `@vendor/shadowquic/shadowquic/src/config/mod.rs`:
- Around line 334-344: Update the CongestionControl PartialEq implementation’s
matches! expression to include a self-pairing case for CongestionControl::Bbr3,
preserving the existing equality behavior for all other variants.
In `@vendor/shadowquic/shadowquic/src/config/serde_utils.rs`:
- Around line 16-18: Update the suffix multiplier mapping in the bandwidth
parser to use decimal SI units: `K`/`k` as 1,000, `M`/`m` as 1,000,000, and
`G`/`g` as 1,000,000,000, while preserving the existing suffix parsing behavior.
In `@vendor/shadowquic/shadowquic/src/config/sunnyquic.rs`:
- Around line 212-213: Update SunnyQuicClientCfg and its serde attributes to
avoid combining deny_unknown_fields with the flattened socket_opt SocketOpt
field, preserving successful deserialization of fields intended for SocketOpt.
In `@vendor/shadowquic/shadowquic/src/direct/outbound.rs`:
- Around line 30-73: Update handle so failures from the async fut are propagated
to its caller instead of being consumed by the spawned task: await the task
(including its JoinError) and return the resulting error through handle’s
anyhow::Result<(), SError>. Preserve the existing direct TCP and UDP handling
and tracing behavior, and remove the fire-and-forget error logging path unless
it remains necessary after propagation.
In `@vendor/shadowquic/shadowquic/src/error.rs`:
- Line 26: Update the error message attribute in the relevant error variant to
remove the duplicated “closed,” changing it to “UDP session closed due to: {0}.”
- Around line 36-44: Remove the commented-out QuicError definition and its
associated rustls_jls::Error conversion implementation from the error module,
including the commented derive and error attributes. Leave the surrounding
active error handling unchanged.
In `@vendor/shadowquic/shadowquic/src/lib.rs`:
- Around line 94-110: Rename the local variable `siz` to `size` in
`Sender<(Bytes, SocksAddr)>::send_to`, updating both its initialization and
return usage without changing behavior.
In `@vendor/shadowquic/shadowquic/src/mixed/inbound.rs`:
- Around line 38-67: Update handle_connection to propagate local_addr() failures
with the existing SResult error flow instead of panicking: replace the unwrap on
stream.local_addr() with ?. Keep the subsequent address conversion and request
handling unchanged.
In `@vendor/shadowquic/shadowquic/src/msgs/mod.rs`:
- Around line 137-142: Update the SEncode implementation for String so encode
writes the string length using the existing length-encoding behavior, then
writes self.as_bytes() directly in a single bulk operation. Remove the to_vec
conversion and avoid routing through Vec<u8>::encode or per-byte u8::encode
calls.
- Around line 127-136: Update the Vec<E> SDecode implementation’s decode method
to validate the network-provided len against a hard maximum before calling
Vec::with_capacity or reading elements; reject oversized lengths using the
existing SError mechanism, while preserving normal decoding for lengths within
the cap and propagating element decode errors.
In `@vendor/shadowquic/shadowquic/src/msgs/socks5.rs`:
- Around line 177-195: Update SocksAddr::to_socket_addrs for
AddrOrDomain::Domain to handle invalid UTF-8 from x.contents without panicking.
Replace the expect-based conversion with error propagation through the method’s
std::io::Result, while preserving normal domain resolution and existing
IPv4/IPv6 behavior.
- Around line 70-77: Update VarVec::from to reject or otherwise explicitly
handle input vectors longer than 255 bytes before converting the length to u8,
preventing truncation and data loss during encode. Apply the same length
validation to SocksAddr::from_domain before storing name.len() as u8, preserving
valid inputs and ensuring oversized credentials or domain names cannot be
silently encoded with an incorrect length.
In `@vendor/shadowquic/shadowquic/src/msgs/squic.rs`:
- Around line 20-21: Rename the enum variants SQAssociatOverDatagram and
SQAssociatOverStream to SQAssociateOverDatagram and SQAssociateOverStream, and
correct the SQExtOpcode documentation typo from “Extention” to “Extension.”
Update all references to the renamed variants so the code remains consistent.
In `@vendor/shadowquic/shadowquic/src/quic/mod.rs`:
- Around line 16-19: Remove the commented-out gm_quic_wrapper module declaration
and its public re-export from the QUIC module declarations; do not replace them
with alternative code.
In `@vendor/shadowquic/shadowquic/src/shadowquic/inbound.rs`:
- Around line 142-171: Update the error logging for the spawned handle_incoming
task to include connection-identifying context, such as the remote address,
alongside the existing error. Preserve the current behavior of consuming the
error so the server continues accepting other connections.
- Around line 189-206: Update the accept loop around QuicServer::accept to add a
small delay or exponential backoff after Err(e) before retrying, while
preserving immediate retries after successful connections and the existing error
logging.
- Around line 120-140: Update ShadowQuicServer::new to propagate errors from
QuicServer::new instead of calling expect, converting the endpoint bind error
into the function’s Result<Self, SError> return type while preserving the
existing successful initialization flow.
In `@vendor/shadowquic/shadowquic/src/shadowquic/outbound.rs`:
- Around line 52-54: Correct the typo in the panic messages within the outbound
address-resolution flow: change “faile” to “failed” at both occurrences and
“connectin” to “connection” at the connection-related message around the same
implementation.
- Around line 47-62: Update get_conn to return SError::DomainResolveFailed(...)
for both socket address resolution failure and missing resolved address instead
of panicking. Replace quic_end.get_or_init with get_or_try_init, propagate the
Result from init_endpoint().await, and preserve the existing connection flow
after successful initialization.
In `@vendor/shadowquic/shadowquic/src/shadowquic/quinn_wrapper/wrapper.rs`:
- Around line 389-420: Update the None branch of quinn_wrapper::accept to return
the existing typed endpoint-closure QuicErrorRepr instead of panicking. In
SunnyQuicServer::init, recognize that terminal error, exit the accept loop
cleanly, and avoid retrying or propagating it as a panic; preserve normal
handling for other accept errors.
- Around line 117-128: Update the `QuicErrorRepr::send_datagram` implementation
so the `SendDatagramError::TooLarge` match arm still logs the oversized payload
but propagates the converted error instead of falling through to `Ok(())`.
Preserve the existing handling for successful sends and other errors.
In `@vendor/shadowquic/shadowquic/src/socks/inbound.rs`:
- Around line 148-154: Update the UDP association flow around the
SOCKS5_CMD_UDP_ASSOCIATE branch and UdpSocksWrap to retain the authenticated TCP
peer address when creating the association. Bind or configure the UDP handling
to accept packets only from that peer, rejecting packets from other source
addresses instead of trusting the first sender; preserve the existing local bind
address and reply behavior.
In `@vendor/shadowquic/shadowquic/src/socks/mod.rs`:
- Around line 23-43: Update the UDP receive/decode flow around recv_from and
socks5::UdpReqHeader::decode to decode only the received len bytes, and ensure
larger datagrams are not silently truncated by the fixed 2000-byte buffer.
Preserve the existing fragmented-packet handling and return payload slicing,
while making truncated headers produce the decoder’s error instead of allowing
an invalid headsize range to panic.
- Around line 35-40: The UdpSocksWrap::recv_from initialization currently
discards UdpSocket::connect errors and caches dst regardless of success. Replace
the self.1.get_or_init initialization with fallible initialization such as
get_or_try_init, propagate the connect error, and return/store dst only after
self.0.connect(dst) succeeds so failed attempts do not mark the peer
initialized.
In `@vendor/shadowquic/shadowquic/src/socks/outbound.rs`:
- Around line 145-153: Validate the decoded SOCKS command reply before treating
the request as established in both TCP and UDP outbound paths. Update the reply
handling around CmdReply::decode in the TCP flow and the corresponding UDP flow
to require SOCKS5_REPLY_SUCCEEDED; otherwise return the existing appropriate
error instead of forwarding or resolving the bind address.
- Line 242: Correct the spelling typo in the comment near the spawn/shutdown
communication note in the outbound SOCKS implementation, changing “requirs” to
“requires” without altering the comment’s meaning.
- Around line 104-119: Update the authentication handling in the outbound
request flow to resolve self.cfg.password once with ok_or before constructing
PasswordAuthReq. Remove the unwrap from the password length expression and reuse
the resolved password for both its length and contents, returning the existing
SocksError when username is present but password is missing.
- Around line 131-142: Update the SOCKS address resolution in handle_tcp and
handle_udp to use Tokio’s asynchronous resolver, such as tokio::net::lookup_host
or the cached resolver pattern from the direct outbound implementation, instead
of self.cfg.addr.to_socket_addrs(). Preserve selecting the first resolved
address and the existing not-found error behavior.
In `@vendor/shadowquic/shadowquic/src/squic/inbound.rs`:
- Around line 63-71: Rename the “bistream” identifiers and log/span text in the
inbound stream handling flow, including handle_bistream, to use “bidirectional
stream” terminology. Rename SQAssociat* enum variants to SQAssociate* in their
declaration and update every caller, including the referenced locations, so
protocol naming remains consistent.
- Around line 181-192: Update handle_user_extension to stop authorizing
administrators via authed_user.starts_with("admin"). Retrieve the authenticated
user’s explicit role or permission from the existing authentication state and
allow user-management operations only when that value grants administrator
privileges; preserve the PermissionDenied response for all other authenticated
accounts.
- Around line 136-156: Replace the repeated-authentication expect in the
SQReq::SQAuthenticate branch with an SError return and close the offending
stream or connection before returning. Implement SQBind handling instead of
allowing the request to reach the wildcard unimplemented! path: reject it with
an appropriate SError and close the connection. Preserve successful first
authentication and supported request handling.
In `@vendor/shadowquic/shadowquic/src/squic/mod.rs`:
- Line 43: Correct the spelling errors in the documentation comments, updating
“destionation” to “destination,” “occured” to “occurred,” and “symetrical” to
“symmetrical,” and replace “bistream” with “bidirectional stream.” Apply these
edits at the referenced comment locations without changing code behavior.
- Around line 190-203: The fetch_new_id method can loop indefinitely when every
u16 context ID is occupied. Track the initial ID and stop after scanning all
65,536 possible IDs without finding an Entry::Vacant slot, then return the
established exhaustion error while preserving successful insertion and ID return
behavior.
- Around line 208-235: Update feed_datagram and the associated
NotifyBuffer/state management to enforce bounds on unresolved IDs, buffered
packet count per ID, and aggregate pending bytes. Track total buffered bytes and
reject or close the connection when any configured limit is exceeded, while
preserving delivery for resolved entries and ensuring counters are decremented
when buffers are flushed or removed.
- Around line 245-249: Update store_socket_with_prelude so every duplicate
context-ID path, including the branches around the referenced match handling,
returns an error immediately instead of logging and continuing. Ensure id_map is
not modified for duplicates, while preserving successful insertion for new IDs.
- Around line 208-219: Update feed_datagram and the related buffered-prelude
loop to release IDStore read/write guards before awaiting socket send_to calls.
Extract or clone the socket and address while holding the lock, explicitly drop
the guard, then perform socket I/O; preserve the existing entry lookup,
registration, cleanup, and dispatch behavior.
In `@vendor/shadowquic/shadowquic/src/squic/outbound.rs`:
- Line 32: Apply the spelling corrections throughout the affected outbound
implementation: rename the tracing span label “bistream” to “bidirectional
stream,” correct every “dstination” occurrence to “destination,” and rename all
SQAssociat* symbols to SQAssociate*, including the related enum declaration and
its usages.
In `@vendor/shadowquic/shadowquic/src/sunnyquic/dynamic_cert.rs`:
- Around line 60-73: Update the certificate watcher setup around
recommended_watcher and both watcher.watch calls to propagate failures through
the function’s Result return instead of panicking with expect. Map each
notify::Error into SError while preserving the existing watcher registration
flow and error context.
- Around line 37-49: Update the certificate parsing in the dynamic certificate
reload flow around CertificateDer::pem_file_iter and the successful update
branch: collect PEM entries as a Result so any malformed certificate propagates
as an error instead of being dropped, and emit the reload success log only after
the certificate/key update completes successfully.
In `@vendor/shadowquic/shadowquic/src/sunnyquic/gm_quic_wrapper.rs`:
- Around line 201-207: Update the listener construction in the QuicListeners
builder chain to call enable_0rtt() only when cfg.zero_rtt is enabled; otherwise
leave 0-RTT disabled while preserving the existing server parameters, ALPNs, and
listen configuration.
- Around line 234-237: Update SunnyQuicServer::update_config so it no longer
returns Ok(()) without applying the supplied configuration. Implement the
configuration update using the existing server state, or return the established
ProtocolUnimpl error when updates are unsupported, while preserving the async
method’s SResult return type.
- Around line 101-106: Replace the panic-prone unwrap calls in connection
creation, listener registration, and connection acceptance with error
propagation through the existing QuicErrorRepr/SError return paths. Update the
connected_to flow and the listener/accept handling near the affected sections,
preserving the current successful Connection construction and task behavior
while returning recoverable errors instead of terminating the task.
- Around line 157-159: Implement the GM QUIC backend’s `close` method instead of
leaving it as `unimplemented!`, ensuring `SQServerConn` invalid-authentication
paths close the connection without panicking. Use the provided `error_code` and
`reason` with the underlying connection’s established close mechanism, matching
the behavior of other backend implementations.
In `@vendor/shadowquic/shadowquic/src/sunnyquic/inbound.rs`:
- Around line 138-140: Update the endpoint initialization in new_inner to
propagate QuicServer::new errors through the constructor’s existing Result
return instead of calling expect and panicking. Preserve the successful
EndServer assignment while allowing bind, TLS, and backend setup failures to
reach the caller.
- Around line 171-189: Update handle_incoming and SQServerConn to retain the
shared ArcSwap credential store rather than a cloned HashMap snapshot. In the
SQAuthenticate processing path, load the current credential map from ArcSwap for
each authentication attempt so password changes and user removal take effect on
existing connections.
- Around line 224-243: Update the accept loop in the async task around
QuicServer::accept to apply bounded exponential backoff after failures,
resetting the delay after a successful connection. Detect terminal endpoint
errors and terminate the listener instead of retrying them; retain error logging
while preventing rapid retries and log flooding.
- Around line 43-79: Refactor the configuration mutation flow used by add_user
and remove_user so endpoint updates, the authentication map, and stored config
are serialized under one shared mutation lock. Update update_config to
participate in that same lock rather than modifying endpoint/user state before
acquiring it, and commit all three states together with rollback on failure.
Preserve the existing duplicate-user and NotFound behavior while ensuring
concurrent mutations cannot publish different revisions.
In `@vendor/shadowquic/shadowquic/src/sunnyquic/mod.rs`:
- Around line 21-45: Gate gen_sunny_user_hash and its digest-dependent
implementation behind the same ring/aws-lc-rs cfg conditions used by the digest
imports, so it is unavailable when neither backend is enabled. Preserve the
existing hash behavior for builds selecting either digest backend.
In `@vendor/shadowquic/shadowquic/src/sunnyquic/noq_wrapper/wrapper.rs`:
- Around line 73-80: Remove the commented-out packet loss rate and diagnostic
logging blocks from both open_bi and accept_bi, leaving the surrounding
executable code unchanged.
- Around line 1-44: Consolidate the scattered imports in the wrapper module by
moving UdpSocket into the existing std::net group, and grouping Rustls
ServerConfig and noq::crypto::rustls types with their related imports. Run cargo
+nightly fmt --all and preserve the formatter’s case-sensitive ASCII ordering.
- Line 238: Correct the spelling in the comment near the multipath server-hello
handling from “knowning” to “knowing”, then run cargo +nightly fmt --all before
committing.
- Line 466: Remove the empty for loop iterating over cfg.users, as it has no
effect. Do not add replacement processing unless required elsewhere; leave the
surrounding wrapper logic unchanged.
- Around line 120-148: Replace the panic-prone paths in `send_datagram`,
`close`, and `accept` with propagation through the existing
`SError`/`QuicErrorRepr` error types. In `send_datagram`, handle a missing
`max_datagram_size()` without unwrapping; in `close`, convert invalid
`error_code` values into the method’s supported error behavior instead of
unwrapping `VarInt::from_u64`; and in `accept`, return an appropriate shutdown
error rather than panicking. Use the existing comprehensive `thiserror`-based
error variants where available.
- Around line 289-300: Update gen_client_cfg to return
SResult<noq::ClientConfig> and replace the load_native_certs().expect(...) and
pem_file_iter panic path with propagated errors. Adjust the client new() call
site to handle the returned result so invalid system roots or a missing
certificate file surface as initialization errors instead of aborting.
In `@vendor/shadowquic/shadowquic/src/sunnyquic/outbound.rs`:
- Around line 53-55: Correct the panic message typos in the outbound address
resolution and connection handling code: update both occurrences of “faile” in
the resolve error messages near the address-resolution chain to “failed,” and
change “connectin” to “connection” in the message near the connection logic.
- Around line 48-63: Update get_conn to propagate failures through Result
instead of panicking: replace the addr DNS resolution
unwrap_or_else/unwrap_or_else chain with SError::DomainResolveFailed, preserving
the configured address in the error, and propagate init_endpoint errors from
quic_end using fallible OnceCell initialization (get_or_try_init if supported).
Keep successful endpoint and connection creation behavior unchanged.
In `@vendor/shadowquic/shadowquic/src/tproxy/inbound.rs`:
- Around line 90-107: Replace the unhandled stream.peer_addr().unwrap() call in
the TCP accept branch with fallible error propagation through the existing
Result flow, converting the I/O error consistently with the surrounding
local_addr handling before logging the peer address. Preserve the accepted
connection processing when peer_addr succeeds.
- Around line 162-201: Update destination address construction in the reply send
path to preserve the complete original self.client_addr for IPv6, including its
port and scope_id, instead of rebuilding it from only the IpAddr. Keep the
existing IPv4 behavior and sendto flow unchanged.
- Around line 19-21: Update the socket2 dependency declaration in Cargo.toml to
enable its all feature while retaining the existing 0.6 version requirement, so
the APIs used by inbound.rs—including MsgHdrMut, MaybeUninitSlice, and
SockAddrStorage—are available.
- Around line 259-315: Cap the `sessions` cache in `handle_udp_tproxy` before
inserting a new `client_addr` session. Add a fixed maximum or LRU-style eviction
that removes an existing entry when the cap is reached, while preserving reuse
of existing sessions and the current closed/idle cleanup behavior.
In `@vendor/shadowquic/shadowquic/src/utils/dual_socket.rs`:
- Around line 13-16: Remove the blanket Deref implementation for DualSocket so
callers cannot bypass its IPv4 and IPv4-mapped-IPv6 translation in send_to and
recv_from. Add explicit passthrough methods only for the inner UdpSocket
operations the surrounding code requires, updating affected call sites to use
those methods while preserving DualSocket’s translation-aware methods.
In `@vendor/shadowquic/shadowquic/src/utils/platform/fallback.rs`:
- Around line 4-7: Update the fallback bind_device function to return an
io::Error indicating that interface binding is unsupported, rather than logging
a warning and returning Ok(()). Preserve the existing function signature and
ensure callers can detect and handle the failed binding attempt.
In `@vendor/shadowquic/shadowquic/src/utils/platform/windows.rs`:
- Around line 10-18: Update the idx == 0 failure path in bind_device to return a
deterministic io::Error with ErrorKind::NotFound, using the requested
device_name in the error message instead of calling io::Error::last_os_error().
In `@vendor/shadowquic/shadowquic/src/utils/socket_opt.rs`:
- Around line 24-30: Update UdpSocketFactory::create_socket and
TcpSocketFactory::create_socket to propagate to_socket_addrs and next resolution
failures as std::io::Result errors instead of panicking, with corrected
“failed”/“fail” wording. Because hostname resolution is blocking, perform it via
spawn_blocking (or the project’s existing blocking-resolution helper) before
continuing socket creation, preserving the resolved-address flow and propagating
join or resolution errors.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 58ec7f87-1669-4eb7-9a26-541bc9e0f52f
📒 Files selected for processing (66)
Cargo.tomlclash-lib/src/proxy/sunnyquic/mod.rssunnyquic-inbound-brutal.yamlsunnyquic-inbound.yamlvendor/shadowquic/.gitignorevendor/shadowquic/CLASH-RS.mdvendor/shadowquic/Cargo.tomlvendor/shadowquic/LICENSEvendor/shadowquic/README.mdvendor/shadowquic/shadowquic-macros/Cargo.tomlvendor/shadowquic/shadowquic-macros/src/lib.rsvendor/shadowquic/shadowquic/Cargo.tomlvendor/shadowquic/shadowquic/src/config/mod.rsvendor/shadowquic/shadowquic/src/config/serde_utils.rsvendor/shadowquic/shadowquic/src/config/shadowquic.rsvendor/shadowquic/shadowquic/src/config/sunnyquic.rsvendor/shadowquic/shadowquic/src/direct/mod.rsvendor/shadowquic/shadowquic/src/direct/outbound.rsvendor/shadowquic/shadowquic/src/error.rsvendor/shadowquic/shadowquic/src/http/inbound.rsvendor/shadowquic/shadowquic/src/http/mod.rsvendor/shadowquic/shadowquic/src/lib.rsvendor/shadowquic/shadowquic/src/main.rsvendor/shadowquic/shadowquic/src/mixed/inbound.rsvendor/shadowquic/shadowquic/src/mixed/mod.rsvendor/shadowquic/shadowquic/src/msgs/mod.rsvendor/shadowquic/shadowquic/src/msgs/socks5.rsvendor/shadowquic/shadowquic/src/msgs/socks5_addr_test.rsvendor/shadowquic/shadowquic/src/msgs/squic.rsvendor/shadowquic/shadowquic/src/observe/imple.rsvendor/shadowquic/shadowquic/src/observe/mod.rsvendor/shadowquic/shadowquic/src/observe/unimpl.rsvendor/shadowquic/shadowquic/src/quic/mod.rsvendor/shadowquic/shadowquic/src/shadowquic/inbound.rsvendor/shadowquic/shadowquic/src/shadowquic/mod.rsvendor/shadowquic/shadowquic/src/shadowquic/outbound.rsvendor/shadowquic/shadowquic/src/shadowquic/quinn_wrapper/brutal.rsvendor/shadowquic/shadowquic/src/shadowquic/quinn_wrapper/mod.rsvendor/shadowquic/shadowquic/src/shadowquic/quinn_wrapper/wrapper.rsvendor/shadowquic/shadowquic/src/socks/inbound.rsvendor/shadowquic/shadowquic/src/socks/mod.rsvendor/shadowquic/shadowquic/src/socks/outbound.rsvendor/shadowquic/shadowquic/src/squic/inbound.rsvendor/shadowquic/shadowquic/src/squic/mod.rsvendor/shadowquic/shadowquic/src/squic/outbound.rsvendor/shadowquic/shadowquic/src/sunnyquic/dynamic_cert.rsvendor/shadowquic/shadowquic/src/sunnyquic/gm_quic_wrapper.rsvendor/shadowquic/shadowquic/src/sunnyquic/inbound.rsvendor/shadowquic/shadowquic/src/sunnyquic/mod.rsvendor/shadowquic/shadowquic/src/sunnyquic/noq_wrapper/brutal.rsvendor/shadowquic/shadowquic/src/sunnyquic/noq_wrapper/mod.rsvendor/shadowquic/shadowquic/src/sunnyquic/noq_wrapper/wrapper.rsvendor/shadowquic/shadowquic/src/sunnyquic/outbound.rsvendor/shadowquic/shadowquic/src/sunnyquic/unimplemented.rsvendor/shadowquic/shadowquic/src/tproxy/inbound.rsvendor/shadowquic/shadowquic/src/tproxy/mod.rsvendor/shadowquic/shadowquic/src/utils/dual_socket.rsvendor/shadowquic/shadowquic/src/utils/mod.rsvendor/shadowquic/shadowquic/src/utils/platform/fallback.rsvendor/shadowquic/shadowquic/src/utils/platform/linux.rsvendor/shadowquic/shadowquic/src/utils/platform/macos.rsvendor/shadowquic/shadowquic/src/utils/platform/mod.rsvendor/shadowquic/shadowquic/src/utils/platform/windows.rsvendor/shadowquic/shadowquic/src/utils/protect_socket.rsvendor/shadowquic/shadowquic/src/utils/replay_stream.rsvendor/shadowquic/shadowquic/src/utils/socket_opt.rs
Comment on lines
+125
to
+130
| Event::Request(Ok(request)) => { | ||
| dispatch_request( | ||
| request, | ||
| unspecified_source(self.addr), | ||
| self.dispatcher.clone(), | ||
| self.fw_mark, |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline clash-lib/src/proxy/sunnyquic/mod.rs \
--match 'dispatch_request|ProxyRequest' --view expanded
ast-grep outline vendor/shadowquic/shadowquic/src/sunnyquic/inbound.rs \
--match 'ProxyRequest|handle_incoming' --view expanded
rg -n -C3 '\b(remote_address|peer_addr|user_context)\b' \
clash-lib/src/proxy/sunnyquic vendor/shadowquic/shadowquic/srcRepository: Watfaq/clash-rs
Length of output: 20298
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== clash-lib/src/proxy/sunnyquic/mod.rs =="
sed -n '1,260p' clash-lib/src/proxy/sunnyquic/mod.rs
echo
echo "== search for unspecified_source and Session::as_map =="
rg -n -C4 'unspecified_source|as_map|source.*addr|peer_addr|remote_address' clash-lib/src clash-lib/vendor vendor/shadowquic/shadowquic/srcRepository: Watfaq/clash-rs
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== dispatch_request in clash-lib/src/proxy/sunnyquic/mod.rs =="
sed -n '260,360p' clash-lib/src/proxy/sunnyquic/mod.rs
echo
echo "== ProxyRequest / SunnyQuic inbound in vendor/shadowquic/shadowquic/src/sunnyquic/inbound.rs =="
sed -n '1,260p' vendor/shadowquic/shadowquic/src/sunnyquic/inbound.rs
echo
echo "== ProxyRequest definition and Session fields in vendor/shadowquic/shadowquic/src/lib.rs =="
sed -n '1,140p' vendor/shadowquic/shadowquic/src/lib.rsRepository: Watfaq/clash-rs
Length of output: 15599
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== sunnyquic request construction sites =="
rg -n -C4 'ProxyRequest::(Tcp|Udp)|TcpSession\s*\{|UdpSession\s*\{' vendor/shadowquic/shadowquic/src/sunnyquic vendor/shadowquic/shadowquic/src/squic
echo
echo "== methods on the quic connection used by sunnyquic =="
rg -n -C3 'remote_address\(|peer_id\(|handle_connection|accept\(' vendor/shadowquic/shadowquic/src/sunnyquic vendor/shadowquic/shadowquic/src/squicRepository: Watfaq/clash-rs
Length of output: 16821
Preserve the client address in dispatched sessions.
This path always falls back to unspecified_source(self.addr), so Session::source is recorded as 0.0.0.0:0/[::]:0 and downstream observability/policy loses the real peer address. Thread the QUIC connection’s remote address through ProxyRequest and use it here.
🤖 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 `@clash-lib/src/proxy/sunnyquic/mod.rs` around lines 125 - 130, Update the
Event::Request handling around dispatch_request to obtain the QUIC connection’s
remote address, thread it through ProxyRequest, and pass that address as the
session source instead of unspecified_source(self.addr). Preserve the real
client address for downstream Session::source observability and policy.
Comment on lines
+1
to
+20
| [workspace] | ||
| members = ["shadowquic", "shadowquic-macros"] | ||
| default-members = ["shadowquic"] | ||
| resolver = "3" | ||
|
|
||
|
|
||
| [workspace.package] | ||
| version = "0.3.11" | ||
| edition = "2024" | ||
| authors = ["spongebob888"] | ||
| categories = ["proxy", "quic"] | ||
| keywords = ["quic", "sni"] | ||
| homepage = "https://github.com/spongebob888/shadowquic" | ||
| repository = "https://github.com/spongebob888/shadowquic.git" | ||
| description = "A 0-RTT QUIC Proxy with SNI camouflage" | ||
| license = "MIT" | ||
| readme = "README.md" | ||
|
|
||
|
|
||
| [workspace.dependencies] |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Configure the required release-size profile.
This standalone workspace lacks the required LTO, stripping, and size optimization settings.
Proposed configuration
[workspace.dependencies]
+
+[profile.release]
+opt-level = "z"
+lto = true
+strip = trueAs per coding guidelines, “Configure release optimization for size optimization with LTO and strip.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| [workspace] | |
| members = ["shadowquic", "shadowquic-macros"] | |
| default-members = ["shadowquic"] | |
| resolver = "3" | |
| [workspace.package] | |
| version = "0.3.11" | |
| edition = "2024" | |
| authors = ["spongebob888"] | |
| categories = ["proxy", "quic"] | |
| keywords = ["quic", "sni"] | |
| homepage = "https://github.com/spongebob888/shadowquic" | |
| repository = "https://github.com/spongebob888/shadowquic.git" | |
| description = "A 0-RTT QUIC Proxy with SNI camouflage" | |
| license = "MIT" | |
| readme = "README.md" | |
| [workspace.dependencies] | |
| [workspace] | |
| members = ["shadowquic", "shadowquic-macros"] | |
| default-members = ["shadowquic"] | |
| resolver = "3" | |
| [workspace.package] | |
| version = "0.3.11" | |
| edition = "2024" | |
| authors = ["spongebob888"] | |
| categories = ["proxy", "quic"] | |
| keywords = ["quic", "sni"] | |
| homepage = "https://github.com/spongebob888/shadowquic" | |
| repository = "https://github.com/spongebob888/shadowquic.git" | |
| description = "A 0-RTT QUIC Proxy with SNI camouflage" | |
| license = "MIT" | |
| readme = "README.md" | |
| [workspace.dependencies] | |
| [profile.release] | |
| opt-level = "z" | |
| lto = true | |
| strip = true |
🤖 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 `@vendor/shadowquic/Cargo.toml` around lines 1 - 20, Update the workspace
configuration in Cargo.toml by adding a release profile with size-oriented
optimization, link-time optimization enabled, and symbol stripping configured,
while leaving the existing workspace package and dependency settings unchanged.
Source: Coding guidelines
Comment on lines
+207
to
+209
| let len = buf.len() as u32; | ||
| len.encode(s).await?; | ||
| s.write_all(&buf).await?; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Bound size-tagged payloads before allocation and framing.
Decode permits a peer-controlled allocation of nearly 4 GiB. Encode also truncates payloads larger than u32::MAX, then writes the full buffer, desynchronizing subsequent frames. Enforce a protocol maximum and use u32::try_from(buf.len()).
Also applies to: 435-439
🤖 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 `@vendor/shadowquic/shadowquic-macros/src/lib.rs` around lines 207 - 209,
Update the size-tagged payload encode/decode paths around len.encode and buffer
allocation to enforce the protocol’s maximum payload size before framing or
allocating. Convert buffer lengths with u32::try_from instead of truncating
casts, reject oversized payloads, and ensure both encode locations and the
corresponding decode path preserve frame synchronization.
Comment on lines
+16
to
+18
| Some(b'K') | Some(b'k') => (&s[..s.len() - 1], 1024f64), | ||
| Some(b'M') | Some(b'm') => (&s[..s.len() - 1], 1024f64 * 1024f64), | ||
| Some(b'G') | Some(b'g') => (&s[..s.len() - 1], 1024f64 * 1024f64 * 1024f64), |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,180p' vendor/shadowquic/shadowquic/src/config/serde_utils.rsRepository: Watfaq/clash-rs
Length of output: 3099
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== references ==\n'
rg -n --hidden --glob '!**/target/**' --glob '!**/node_modules/**' \
'parse_bps|deserialize_bps|bandwidth|100M|1\.5G|30M|bps|K=|M=|G=' .
printf '\n== candidate files ==\n'
git ls-files | rg '(^|/)(README|readme|docs|.*config.*|.*serde_utils.*|.*shadowquic.*|.*clash.*)\.(md|rst|txt|rs|toml|yaml|yml)$'Repository: Watfaq/clash-rs
Length of output: 33179
🏁 Script executed:
sed -n '280,340p' vendor/shadowquic/shadowquic/src/config/mod.rs && printf '\n---\n' && sed -n '1,120p' vendor/shadowquic/README.md && printf '\n---\n' && sed -n '1,120p' vendor/shadowquic/CLASH-RS.mdRepository: Watfaq/clash-rs
Length of output: 4234
🏁 Script executed:
rg -n --hidden --glob '!**/target/**' --glob '!**/node_modules/**' \
'1024f64|100M|1\.5G|30M|accepted|bps' vendor/shadowquic/shadowquic/src clash-lib/src clash-bin/tests/data/config sunnyquic-inbound*.yamlRepository: Watfaq/clash-rs
Length of output: 11833
Use decimal multipliers for K/M/G
100M is documented as accepted bandwidth input, but these suffixes currently use 1024-based multipliers, so 100M becomes 104,857,600 bps instead of 100,000,000. Switch this parser to SI units (1_000, 1_000_000, 1_000_000_000) to match the config’s bps wording.
🤖 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 `@vendor/shadowquic/shadowquic/src/config/serde_utils.rs` around lines 16 - 18,
Update the suffix multiplier mapping in the bandwidth parser to use decimal SI
units: `K`/`k` as 1,000, `M`/`m` as 1,000,000, and `G`/`g` as 1,000,000,000,
while preserving the existing suffix parsing behavior.
Comment on lines
+212
to
+213
| #[serde(flatten)] | ||
| pub socket_opt: SocketOpt, |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major
Same deny_unknown_fields + #[serde(flatten)] concern as ShadowQuicClientCfg.
SunnyQuicClientCfg uses deny_unknown_fields (line 142) with #[serde(flatten)] pub socket_opt: SocketOpt (lines 212-213). Fields intended for SocketOpt may be rejected as unknown by the parent struct. See the same issue flagged in shadowquic.rs.
🤖 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 `@vendor/shadowquic/shadowquic/src/config/sunnyquic.rs` around lines 212 - 213,
Update SunnyQuicClientCfg and its serde attributes to avoid combining
deny_unknown_fields with the flattened socket_opt SocketOpt field, preserving
successful deserialization of fields intended for SocketOpt.
Comment on lines
+259
to
+315
| let mut sessions: HashMap<SocketAddr, (Sender<(Bytes, SocksAddr)>, std::time::Instant)> = | ||
| HashMap::new(); | ||
| const MAX_UDP_PAYLOAD_SIZE: usize = 65536; | ||
| const UDP_GRO_MAX_SEGMENTS: usize = 64; | ||
|
|
||
| let mut buf = vec![MaybeUninit::uninit(); MAX_UDP_PAYLOAD_SIZE * UDP_GRO_MAX_SEGMENTS]; | ||
| let idle_timeout = std::time::Duration::from_secs(300); | ||
| let mut cleanup_interval = tokio::time::interval(std::time::Duration::from_secs(60)); | ||
|
|
||
| loop { | ||
| tokio::select! { | ||
| recv_result = recv_tproxy_udp_msg(&listener, &mut buf) => { | ||
| match recv_result { | ||
| Ok(meta) => { | ||
| let orig_dst = match meta.orig_dst { | ||
| Some(d) => d.to_canonical(), | ||
| None => continue, | ||
| }; | ||
|
|
||
| let client_addr = meta.addr.to_canonical(); | ||
| let now = std::time::Instant::now(); | ||
|
|
||
| sessions.retain(|_, (tx, _)| !tx.is_closed()); | ||
|
|
||
| let tx = if let Some((tx, last_active)) = sessions.get_mut(&client_addr) { | ||
| *last_active = now; | ||
| tx.clone() | ||
| } else { | ||
| tracing::info!("accepted udp connection from {}", client_addr); | ||
| let (tx, rx) = channel(1024); | ||
| let send = Arc::new(TproxyUdpSend { | ||
| client_addr, | ||
| v4_socket: v4_socket.clone(), | ||
| v6_socket: v6_socket.clone(), | ||
| }); | ||
|
|
||
| let req: ProxyRequest<AnyTcp, AnyUdpRecv, AnyUdpSend> = | ||
| ProxyRequest::Udp(UdpSession { | ||
| send: send as Arc<dyn UdpSend>, | ||
| recv: Box::new(rx) as Box<dyn UdpRecv>, | ||
| stream: None, | ||
| bind_addr: SocksAddr { | ||
| addr: match orig_dst.ip() { | ||
| std::net::IpAddr::V4(_) => AddrOrDomain::V4([0, 0, 0, 0]), | ||
| std::net::IpAddr::V6(_) => AddrOrDomain::V6([0u8; 16]), | ||
| }, | ||
| port: 0, | ||
| }, | ||
| user_context: None, | ||
| }); | ||
|
|
||
| if req_tx.send(req).await.is_err() { | ||
| break; | ||
| } | ||
| sessions.insert(client_addr, (tx.clone(), now)); | ||
| tx | ||
| }; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="vendor/shadowquic/shadowquic/src/tproxy/inbound.rs"
echo '--- file outline ---'
ast-grep outline "$FILE" --view expanded || true
echo '--- relevant line ranges ---'
nl -ba "$FILE" | sed -n '240,380p'
echo '--- search for session limits / cleanup / related logic ---'
rg -n "MAX_UDP_SESSIONS|sessions\.retain|cleanup_interval|idle_timeout|channel\\(|req_tx\\.send|handle_udp_tproxy|recv_tproxy_udp_msg" "$FILE"Repository: Watfaq/clash-rs
Length of output: 2279
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="vendor/shadowquic/shadowquic/src/tproxy/inbound.rs"
echo '--- handle_udp_tproxy body ---'
sed -n '204,380p' "$FILE" | cat -n
echo '--- session-related matches ---'
rg -n "sessions|cleanup_interval|idle_timeout|channel\\(|retain\\(|req_tx\\.send|MAX_UDP" "$FILE"Repository: Watfaq/clash-rs
Length of output: 9181
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where the UDP request channel is created/consumed and whether there's any session cap elsewhere.
rg -n "udp_req_tx|udp_req_rx|ProxyRequest::Udp|MAX_.*SESSION|rate limit|limit.*session|session.*limit|HashMap<SocketAddr" vendor/shadowquic/shadowquic/src -g '!**/target/**'
echo '--- TproxyServer::new and accept path ---'
sed -n '1,120p' vendor/shadowquic/shadowquic/src/tproxy/inbound.rs | cat -nRepository: Watfaq/clash-rs
Length of output: 6488
Cap the UDP session cache
handle_udp_tproxy still has no hard bound on sessions; it only drops closed peers immediately and idle peers on the 60s timer after 300s. A burst of distinct source addresses can still accumulate many entries in that window, so add a fixed cap or LRU-style eviction before inserting new sessions.
🤖 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 `@vendor/shadowquic/shadowquic/src/tproxy/inbound.rs` around lines 259 - 315,
Cap the `sessions` cache in `handle_udp_tproxy` before inserting a new
`client_addr` session. Add a fixed maximum or LRU-style eviction that removes an
existing entry when the cap is reached, while preserving reuse of existing
sessions and the current closed/idle cleanup behavior.
Comment on lines
+4
to
+7
| pub fn bind_device(_socket: &Socket, _device_name: &str) -> io::Result<()> { | ||
| tracing::warn!("bind interface not supported on this platform"); | ||
| Ok(()) | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Fail-open interface binding on unsupported platforms.
Unlike Linux/macOS/Windows (which return a hard error when bind_device fails), this fallback silently succeeds without actually binding to the interface, only logging a warn!. Since interface binding can be used for traffic-isolation/leak-prevention purposes, a silent no-op could let traffic leak through an unintended interface on platforms that hit this fallback, with no way for the caller to detect and act on the misconfiguration.
Consider failing loudly instead, consistent with other platforms:
🔒️ Proposed fix to fail closed instead of open
pub fn bind_device(_socket: &Socket, _device_name: &str) -> io::Result<()> {
- tracing::warn!("bind interface not supported on this platform");
- Ok(())
+ tracing::error!("interface binding requested but not supported on this platform");
+ Err(io::Error::new(
+ io::ErrorKind::Unsupported,
+ "binding to a network interface is not supported on this platform",
+ ))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub fn bind_device(_socket: &Socket, _device_name: &str) -> io::Result<()> { | |
| tracing::warn!("bind interface not supported on this platform"); | |
| Ok(()) | |
| } | |
| pub fn bind_device(_socket: &Socket, _device_name: &str) -> io::Result<()> { | |
| tracing::error!("interface binding requested but not supported on this platform"); | |
| Err(io::Error::new( | |
| io::ErrorKind::Unsupported, | |
| "binding to a network interface is not supported on this platform", | |
| )) | |
| } |
🤖 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 `@vendor/shadowquic/shadowquic/src/utils/platform/fallback.rs` around lines 4 -
7, Update the fallback bind_device function to return an io::Error indicating
that interface binding is unsupported, rather than logging a warning and
returning Ok(()). Preserve the existing function signature and ensure callers
can detect and handle the failed binding attempt.
Comment on lines
+10
to
+18
| pub fn bind_device(socket: &Socket, device_name: &str) -> io::Result<()> { | ||
| let c_device_name = std::ffi::CString::new(device_name) | ||
| .map_err(|e| io::Error::new(io::ErrorKind::InvalidInput, e))?; | ||
|
|
||
| let pcstr = PCSTR::from_raw(c_device_name.as_ptr() as *const u8); | ||
| let idx = unsafe { windows::Win32::NetworkManagement::IpHelper::if_nametoindex(pcstr) }; | ||
| if idx == 0 { | ||
| return Err(io::Error::last_os_error()); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Windows if_nametoindex set a retrievable GetLastError code on failure?
💡 Result:
No, the Windows if_nametoindex function does not set a retrievable GetLastError code on failure [1][2][3]. Official Microsoft documentation explicitly states that when the if_nametoindex function fails and returns zero, it is not possible to determine an error code [1][2]. Because the function does not set the last-error code, developers cannot rely on GetLastError to diagnose why the operation failed [4][5]. If you require robust error handling for network interface lookups on Windows, Microsoft recommends using the ConvertInterface family of functions instead, specifically converting the interface name to a LUID using ConvertInterfaceNameToLuidA and subsequently using that LUID to retrieve the index via ConvertInterfaceLuidToIndex [3][6].
Citations:
- 1: https://learn.microsoft.com/en-us/windows/win32/api/netioapi/nf-netioapi-if_nametoindex
- 2: https://learn.microsoft.com/en-us/windows-hardware/drivers/network/if-nametoindex
- 3: https://github.com/MicrosoftDocs/sdk-api/blob/docs/sdk-api-src/content/netioapi/nf-netioapi-if_nametoindex.md
- 4: https://www.mail-archive.com/dev@apr.apache.org/msg29669.html
- 5:
socket.if_nametoindex('no-such-iface')raisesOSError()without settingerrnopython/cpython#100218 - 6: https://ntdoc.m417z.com/if_nametoindex
Return a deterministic error when if_nametoindex fails.
if_nametoindex does not provide a retrievable last-error value on Windows, so io::Error::last_os_error() can report stale or unrelated context. Use io::ErrorKind::NotFound with the interface name instead.
🐛 Proposed fix
let idx = unsafe { windows::Win32::NetworkManagement::IpHelper::if_nametoindex(pcstr) };
if idx == 0 {
- return Err(io::Error::last_os_error());
+ return Err(io::Error::new(
+ io::ErrorKind::NotFound,
+ format!("network interface '{device_name}' not found"),
+ ));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub fn bind_device(socket: &Socket, device_name: &str) -> io::Result<()> { | |
| let c_device_name = std::ffi::CString::new(device_name) | |
| .map_err(|e| io::Error::new(io::ErrorKind::InvalidInput, e))?; | |
| let pcstr = PCSTR::from_raw(c_device_name.as_ptr() as *const u8); | |
| let idx = unsafe { windows::Win32::NetworkManagement::IpHelper::if_nametoindex(pcstr) }; | |
| if idx == 0 { | |
| return Err(io::Error::last_os_error()); | |
| } | |
| pub fn bind_device(socket: &Socket, device_name: &str) -> io::Result<()> { | |
| let c_device_name = std::ffi::CString::new(device_name) | |
| .map_err(|e| io::Error::new(io::ErrorKind::InvalidInput, e))?; | |
| let pcstr = PCSTR::from_raw(c_device_name.as_ptr() as *const u8); | |
| let idx = unsafe { windows::Win32::NetworkManagement::IpHelper::if_nametoindex(pcstr) }; | |
| if idx == 0 { | |
| return Err(io::Error::new( | |
| io::ErrorKind::NotFound, | |
| format!("network interface '{device_name}' not found"), | |
| )); | |
| } |
🤖 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 `@vendor/shadowquic/shadowquic/src/utils/platform/windows.rs` around lines 10 -
18, Update the idx == 0 failure path in bind_device to return a deterministic
io::Error with ErrorKind::NotFound, using the requested device_name in the error
message instead of calling io::Error::last_os_error().
Comment on lines
+24
to
+30
| async fn create_socket(&self) -> std::io::Result<socket2::Socket> { | ||
| let addr = self | ||
| .addr | ||
| .to_socket_addrs() | ||
| .unwrap_or_else(|_| panic!("resolve quic addr faile: {}", self.addr)) | ||
| .next() | ||
| .unwrap_or_else(|| panic!("resolve quic addr faile: {}", self.addr)); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Replace panic! on address resolution failure with a proper io::Result error, and fix the "faile" typo.
Both UdpSocketFactory::create_socket and TcpSocketFactory::create_socket call .to_socket_addrs().unwrap_or_else(|_| panic!(...)) and .next().unwrap_or_else(|| panic!(...)). Since this function already returns std::io::Result<Socket>, a resolution failure (e.g. a misconfigured or transiently-unresolvable listen/bind address) should be returned as Err, not panic the task. If addr can be a hostname, resolution here is also a blocking getaddrinfo() call made directly on the async task without spawn_blocking, which can stall the executor thread. The CI typos check also flags "faile" (should be "failed"/"fail") in these panic messages.
🐛 Proposed fix
- let addr = self
- .addr
- .to_socket_addrs()
- .unwrap_or_else(|_| panic!("resolve quic addr faile: {}", self.addr))
- .next()
- .unwrap_or_else(|| panic!("resolve quic addr faile: {}", self.addr));
+ let addr = self
+ .addr
+ .to_socket_addrs()?
+ .next()
+ .ok_or_else(|| io::Error::new(io::ErrorKind::AddrNotAvailable, format!("could not resolve quic addr: {}", self.addr)))?;(mirror for the TCP factory at Line 118-124, replacing "resolve tcp addr faile")
Also applies to: 118-124
🧰 Tools
🪛 GitHub Actions: CI / Build _ prepare
[error] 28-28: typos: faile should be failed, fail (panic message: "resolve quic addr faile").
[error] 30-30: typos: faile should be failed, fail (panic message: "resolve quic addr faile").
🤖 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 `@vendor/shadowquic/shadowquic/src/utils/socket_opt.rs` around lines 24 - 30,
Update UdpSocketFactory::create_socket and TcpSocketFactory::create_socket to
propagate to_socket_addrs and next resolution failures as std::io::Result errors
instead of panicking, with corrected “failed”/“fail” wording. Because hostname
resolution is blocking, perform it via spawn_blocking (or the project’s existing
blocking-resolution helper) before continuing socket creation, preserving the
resolved-address flow and propagating join or resolution errors.
Source: Pipeline failures
Comment on lines
+334
to
+344
| impl PartialEq for CongestionControl { | ||
| fn eq(&self, other: &Self) -> bool { | ||
| matches!( | ||
| (self, other), | ||
| (CongestionControl::Bbr, CongestionControl::Bbr) | ||
| | (CongestionControl::Cubic, CongestionControl::Cubic) | ||
| | (CongestionControl::NewReno, CongestionControl::NewReno) | ||
| | (CongestionControl::Brutal(_), CongestionControl::Brutal(_)) | ||
| ) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Bbr3 variant missing from PartialEq implementation.
The CongestionControl enum includes Bbr3 (line 331), but the manual PartialEq impl does not match it. This means CongestionControl::Bbr3 == CongestionControl::Bbr3 returns false, violating reflexivity. Any equality-based config comparison or change detection involving Bbr3 will produce incorrect results.
🐛 Proposed fix: add Bbr3 to PartialEq
impl PartialEq for CongestionControl {
fn eq(&self, other: &Self) -> bool {
matches!(
(self, other),
(CongestionControl::Bbr, CongestionControl::Bbr)
| (CongestionControl::Cubic, CongestionControl::Cubic)
| (CongestionControl::NewReno, CongestionControl::NewReno)
- | (CongestionControl::Brutal(_), CongestionControl::Brutal(_))
+ | (CongestionControl::Brutal(_), CongestionControl::Brutal(_))
+ | (CongestionControl::Bbr3, CongestionControl::Bbr3)
)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| impl PartialEq for CongestionControl { | |
| fn eq(&self, other: &Self) -> bool { | |
| matches!( | |
| (self, other), | |
| (CongestionControl::Bbr, CongestionControl::Bbr) | |
| | (CongestionControl::Cubic, CongestionControl::Cubic) | |
| | (CongestionControl::NewReno, CongestionControl::NewReno) | |
| | (CongestionControl::Brutal(_), CongestionControl::Brutal(_)) | |
| ) | |
| } | |
| } | |
| impl PartialEq for CongestionControl { | |
| fn eq(&self, other: &Self) -> bool { | |
| matches!( | |
| (self, other), | |
| (CongestionControl::Bbr, CongestionControl::Bbr) | |
| | (CongestionControl::Cubic, CongestionControl::Cubic) | |
| | (CongestionControl::NewReno, CongestionControl::NewReno) | |
| | (CongestionControl::Brutal(_), CongestionControl::Brutal(_)) | |
| | (CongestionControl::Bbr3, CongestionControl::Bbr3) | |
| ) | |
| } | |
| } |
🤖 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 `@vendor/shadowquic/shadowquic/src/config/mod.rs` around lines 334 - 344,
Update the CongestionControl PartialEq implementation’s matches! expression to
include a self-pairing case for CongestionControl::Bbr3, preserving the existing
equality behavior for all other variants.
| DomainResolveFailed(String), | ||
| #[error("mpsc channel error: {0}")] | ||
| ChannelError(String), | ||
| #[error("UDP session closed closed due to: {0}")] |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix duplicate word in error message.
"UDP session closed closed due to" should be "UDP session closed due to".
🔧 Proposed fix
- #[error("UDP session closed closed due to: {0}")]
+ #[error("UDP session closed due to: {0}")]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[error("UDP session closed closed due to: {0}")] | |
| #[error("UDP session closed due to: {0}")] |
🤖 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 `@vendor/shadowquic/shadowquic/src/error.rs` at line 26, Update the error
message attribute in the relevant error variant to remove the duplicated
“closed,” changing it to “UDP session closed due to: {0}.”
Comment on lines
+36
to
+44
| // #[derive(Error, Debug)] | ||
| // #[error(transparent)] | ||
| // pub struct QuicError(#[from] QuicErrorRepr); | ||
|
|
||
| // impl From<rustls_jls::Error> for SError { | ||
| // fn from(err: rustls_jls::Error) -> Self { | ||
| // SError::RustlsError(err.to_string()) | ||
| // } | ||
| // } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Remove commented-out dead code.
The commented-out QuicError struct and From<rustls_jls::Error> impl serve no purpose and will rot. Remove them.
♻️ Proposed fix
pub type SResult<T> = result::Result<T, SError>;
-
-// #[derive(Error, Debug)]
-// #[error(transparent)]
-// pub struct QuicError(#[from] QuicErrorRepr);
-
-// impl From<rustls_jls::Error> for SError {
-// fn from(err: rustls_jls::Error) -> Self {
-// SError::RustlsError(err.to_string())
-// }
-// }
-
impl From<quinn::rustls::Error> for SError {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // #[derive(Error, Debug)] | |
| // #[error(transparent)] | |
| // pub struct QuicError(#[from] QuicErrorRepr); | |
| // impl From<rustls_jls::Error> for SError { | |
| // fn from(err: rustls_jls::Error) -> Self { | |
| // SError::RustlsError(err.to_string()) | |
| // } | |
| // } | |
| pub type SResult<T> = result::Result<T, SError>; | |
| impl From<quinn::rustls::Error> for SError { |
🤖 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 `@vendor/shadowquic/shadowquic/src/error.rs` around lines 36 - 44, Remove the
commented-out QuicError definition and its associated rustls_jls::Error
conversion implementation from the error module, including the commented derive
and error attributes. Leave the surrounding active error handling unchanged.
Comment on lines
+94
to
+110
| #[async_trait] | ||
| impl UdpSend for Sender<(Bytes, SocksAddr)> { | ||
| async fn send_to(&self, buf: Bytes, addr: SocksAddr) -> Result<usize, SError> { | ||
| let siz = buf.len(); | ||
| self.send((buf, addr)) | ||
| .await | ||
| .map_err(|_| SError::InboundUnavailable)?; | ||
| Ok(siz) | ||
| } | ||
| } | ||
| #[async_trait] | ||
| impl UdpRecv for Receiver<(Bytes, SocksAddr)> { | ||
| async fn recv_from(&mut self) -> Result<(Bytes, SocksAddr), SError> { | ||
| let r = self.recv().await.ok_or(SError::OutboundUnavailable)?; | ||
| Ok(r) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Rename siz to size to fix typos checker pipeline failure.
The pipeline's typos checker flags siz as a likely typo. Rename to size for clarity and to unblock CI.
✏️ Proposed fix
impl UdpSend for Sender<(Bytes, SocksAddr)> {
async fn send_to(&self, buf: Bytes, addr: SocksAddr) -> Result<usize, SError> {
- let siz = buf.len();
+ let size = buf.len();
self.send((buf, addr))
.await
.map_err(|_| SError::InboundUnavailable)?;
- Ok(siz)
+ Ok(size)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[async_trait] | |
| impl UdpSend for Sender<(Bytes, SocksAddr)> { | |
| async fn send_to(&self, buf: Bytes, addr: SocksAddr) -> Result<usize, SError> { | |
| let siz = buf.len(); | |
| self.send((buf, addr)) | |
| .await | |
| .map_err(|_| SError::InboundUnavailable)?; | |
| Ok(siz) | |
| } | |
| } | |
| #[async_trait] | |
| impl UdpRecv for Receiver<(Bytes, SocksAddr)> { | |
| async fn recv_from(&mut self) -> Result<(Bytes, SocksAddr), SError> { | |
| let r = self.recv().await.ok_or(SError::OutboundUnavailable)?; | |
| Ok(r) | |
| } | |
| } | |
| #[async_trait] | |
| impl UdpSend for Sender<(Bytes, SocksAddr)> { | |
| async fn send_to(&self, buf: Bytes, addr: SocksAddr) -> Result<usize, SError> { | |
| let size = buf.len(); | |
| self.send((buf, addr)) | |
| .await | |
| .map_err(|_| SError::InboundUnavailable)?; | |
| Ok(size) | |
| } | |
| } | |
| #[async_trait] | |
| impl UdpRecv for Receiver<(Bytes, SocksAddr)> { | |
| async fn recv_from(&mut self) -> Result<(Bytes, SocksAddr), SError> { | |
| let r = self.recv().await.ok_or(SError::OutboundUnavailable)?; | |
| Ok(r) | |
| } | |
| } |
🧰 Tools
🪛 GitHub Actions: CI / Build _ prepare
[error] 97-97: typos: siz should be six, size (let siz = buf.len()).
[error] 101-101: typos: siz should be six, size (Ok(siz)).
🤖 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 `@vendor/shadowquic/shadowquic/src/lib.rs` around lines 94 - 110, Rename the
local variable `siz` to `size` in `Sender<(Bytes, SocksAddr)>::send_to`,
updating both its initialization and return usage without changing behavior.
Source: Pipeline failures
Comment on lines
+38
to
+67
| async fn handle_connection( | ||
| mut stream: TcpStream, | ||
| http: Arc<HttpProxyServer>, | ||
| users: Arc<Vec<AuthUser>>, | ||
| sender: Sender<ProxyRequest>, | ||
| ) -> SResult<()> { | ||
| use tokio::io::AsyncReadExt; | ||
|
|
||
| let local_addr = to_ipv4_mapped(stream.local_addr().unwrap()); | ||
| let first_byte = stream.read_u8().await?; | ||
|
|
||
| let req = if first_byte == 0x05 { | ||
| let prefix = vec![first_byte]; | ||
| SocksServer::accept_stream_with_local_addr( | ||
| ReplayStream::new(prefix, stream), | ||
| local_addr, | ||
| &users, | ||
| ) | ||
| .await? | ||
| } else { | ||
| let prefix = vec![first_byte]; | ||
| http.accept_stream(ReplayStream::new(prefix, stream)) | ||
| .await? | ||
| }; | ||
|
|
||
| sender | ||
| .send(req) | ||
| .await | ||
| .map_err(|_| SError::ChannelError("mixed request channel closed".into())) | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Replace stream.local_addr().unwrap() with ? to prevent panic.
local_addr() can fail on a closed or errored socket. The unwrap() will panic the spawned task, bypassing the caller's map_err error handling. Since read_u8().await? on the next line already converts io::Error to SError, the same ? operator works here.
🛡️ Proposed fix
async fn handle_connection(
mut stream: TcpStream,
http: Arc<HttpProxyServer>,
users: Arc<Vec<AuthUser>>,
sender: Sender<ProxyRequest>,
) -> SResult<()> {
use tokio::io::AsyncReadExt;
- let local_addr = to_ipv4_mapped(stream.local_addr().unwrap());
+ let local_addr = to_ipv4_mapped(stream.local_addr()?);
let first_byte = stream.read_u8().await?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async fn handle_connection( | |
| mut stream: TcpStream, | |
| http: Arc<HttpProxyServer>, | |
| users: Arc<Vec<AuthUser>>, | |
| sender: Sender<ProxyRequest>, | |
| ) -> SResult<()> { | |
| use tokio::io::AsyncReadExt; | |
| let local_addr = to_ipv4_mapped(stream.local_addr().unwrap()); | |
| let first_byte = stream.read_u8().await?; | |
| let req = if first_byte == 0x05 { | |
| let prefix = vec![first_byte]; | |
| SocksServer::accept_stream_with_local_addr( | |
| ReplayStream::new(prefix, stream), | |
| local_addr, | |
| &users, | |
| ) | |
| .await? | |
| } else { | |
| let prefix = vec![first_byte]; | |
| http.accept_stream(ReplayStream::new(prefix, stream)) | |
| .await? | |
| }; | |
| sender | |
| .send(req) | |
| .await | |
| .map_err(|_| SError::ChannelError("mixed request channel closed".into())) | |
| } | |
| async fn handle_connection( | |
| mut stream: TcpStream, | |
| http: Arc<HttpProxyServer>, | |
| users: Arc<Vec<AuthUser>>, | |
| sender: Sender<ProxyRequest>, | |
| ) -> SResult<()> { | |
| use tokio::io::AsyncReadExt; | |
| let local_addr = to_ipv4_mapped(stream.local_addr()?); | |
| let first_byte = stream.read_u8().await?; | |
| let req = if first_byte == 0x05 { | |
| let prefix = vec![first_byte]; | |
| SocksServer::accept_stream_with_local_addr( | |
| ReplayStream::new(prefix, stream), | |
| local_addr, | |
| &users, | |
| ) | |
| .await? | |
| } else { | |
| let prefix = vec![first_byte]; | |
| http.accept_stream(ReplayStream::new(prefix, stream)) | |
| .await? | |
| }; | |
| sender | |
| .send(req) | |
| .await | |
| .map_err(|_| SError::ChannelError("mixed request channel closed".into())) | |
| } |
🤖 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 `@vendor/shadowquic/shadowquic/src/mixed/inbound.rs` around lines 38 - 67,
Update handle_connection to propagate local_addr() failures with the existing
SResult error flow instead of panicking: replace the unwrap on
stream.local_addr() with ?. Keep the subsequent address conversion and request
handling unchanged.
| let path_addr = addrs_iter.next().ok_or_else(|| { | ||
| QuicErrorRepr::QuicConnect(format!("no valid socket address found for {}", path)) | ||
| })?; | ||
| // We must wait for server hello before knowning whether multipath is enabled |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix typo: "knowning" → "knowing".
Pipeline CI flags this typo. As per coding guidelines, run cargo +nightly fmt --all before committing.
🔧 Proposed fix
- // We must wait for server hello before knowning whether multipath is enabled
+ // We must wait for server hello before knowing whether multipath is enabled📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // We must wait for server hello before knowning whether multipath is enabled | |
| // We must wait for server hello before knowing whether multipath is enabled |
🧰 Tools
🪛 GitHub Actions: CI / Build _ prepare
[error] 238-238: typos: knowning should be knowing (comment).
🤖 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 `@vendor/shadowquic/shadowquic/src/sunnyquic/noq_wrapper/wrapper.rs` at line
238, Correct the spelling in the comment near the multipath server-hello
handling from “knowning” to “knowing”, then run cargo +nightly fmt --all before
committing.
Sources: Coding guidelines, Pipeline failures
Comment on lines
+289
to
+300
| let mut root_store = RootCertStore::empty(); | ||
| for cert in | ||
| rustls_native_certs::load_native_certs().expect("failed to load OS root certificates") | ||
| { | ||
| root_store.add(cert).unwrap(); | ||
| } | ||
|
|
||
| if let Some(path) = &cfg.cert_path { | ||
| let der_cert = CertificateDer::pem_file_iter(path) | ||
| .unwrap_or_else(|_| panic!("certificate not found:{:?}", path)) | ||
| .filter_map(|x| x.ok()); | ||
| root_store.add_parsable_certificates(der_cert); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the rustls-native-certs version in Cargo.toml and its API
fd "Cargo.toml" --exec grep -l "rustls-native-certs" {} \;
rg "rustls-native-certs" --type toml -C2
# Check if load_native_certs returns Result or CertificateResult
fd -t f "Cargo.lock" --exec grep -A2 'name = "rustls-native-certs"' {} \;Repository: Watfaq/clash-rs
Length of output: 334
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Cargo.toml dependency declaration ---'
sed -n '1,220p' vendor/shadowquic/shadowquic/Cargo.toml | rg -n -C2 'rustls-native-certs|rustls'
echo
echo '--- Cargo.lock entries ---'
rg -n -C3 'name = "rustls-native-certs"|rustls-native-certs' -g 'Cargo.lock' .
echo
echo '--- wrapper.rs around the cited lines ---'
sed -n '280,310p' vendor/shadowquic/shadowquic/src/sunnyquic/noq_wrapper/wrapper.rs
echo
echo '--- search vendored source for load_native_certs ---'
rg -n -C3 'fn load_native_certs|load_native_certs\(' vendor/shadowquic -g '*.rs'Repository: Watfaq/clash-rs
Length of output: 6013
🌐 Web query:
rustls-native-certs 0.8.3 load_native_certs CertificateResult docs
💡 Result:
In rustls-native-certs version 0.8.3, the load_native_certs() function returns a CertificateResult struct [1]. This is a departure from earlier versions that returned a Result<Vec<CertificateDer<'static>>, std::io::Error> [2][3]. The CertificateResult struct is defined as follows [1]: #[non_exhaustive] #[derive(Debug, Default)] pub struct CertificateResult { pub certs: Vec<CertificateDer<'static>>, pub errors: Vec, } This change allows the function to return all successfully loaded certificates in the certs field, while simultaneously reporting any errors encountered during the loading process in the errors field [1][4]. This design addresses issues where system misconfigurations might cause partial failures, giving callers the flexibility to decide how to handle those errors [4]. The CertificateResult struct provides convenience methods such as .expect(msg) and .unwrap() to facilitate access to the certificates [1]. These methods will return the Vec<CertificateDer<'static>> if no errors are present, or panic if the errors vector is not empty [1]. It is important to note that load_native_certs() can be an expensive operation, as it may involve reading and parsing files from disk (up to ~300KB) depending on the platform, and therefore should be called sparingly [5][1]. Additionally, the function prioritizes the SSL_CERT_FILE and SSL_CERT_DIR environment variables if they are set [5][1]. If they are not set, it falls back to the platform-specific certificate stores (Windows store, macOS keychain, or system CA bundles via openssl-probe on Linux) [2][3][6].
Citations:
- 1: https://docs.rs/rustls-native-certs/latest/src/rustls_native_certs/lib.rs.html
- 2: https://docs.rs/crate/rustls-native-certs/0.8.3
- 3: https://crates.io/crates/rustls-native-certs/0.8.3
- 4: Yield all the errors rustls/rustls-native-certs#128
- 5: https://docs.rs/rustls-native-certs/latest/rustls_native_certs/fn.load_native_certs.html
- 6: https://docs.rs/crate/rustls-native-certs/latest
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- gen_client_cfg signature and nearby functions ---'
rg -n -C4 'pub fn gen_client_cfg|async fn new|-> crate::error::SResult|-> SResult<|panic!\(' vendor/shadowquic/shadowquic/src/sunnyquic -g '*.rs'
echo
echo '--- error type definitions ---'
rg -n -C3 'type SResult|pub type SResult|enum .*Error|struct .*Error' vendor/shadowquic/shadowquic/src -g '*.rs'
echo
echo '--- other config builders returning Result ---'
rg -n -C3 'ClientConfig|ServerConfig|RootCertStore|add_parsable_certificates|load_native_certs' vendor/shadowquic/shadowquic/src -g '*.rs'Repository: Watfaq/clash-rs
Length of output: 44675
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'gen_client_cfg\s*\(' vendor/shadowquic/shadowquic/src -g '*.rs'Repository: Watfaq/clash-rs
Length of output: 704
Return an error instead of aborting client setup
load_native_certs().expect(...) and the panic! on pem_file_iter still crash initialization on bad system roots or a missing cert file. gen_client_cfg is only called from the client new() path, so returning SResult<noq::ClientConfig> here would let those failures surface cleanly.
🤖 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 `@vendor/shadowquic/shadowquic/src/sunnyquic/noq_wrapper/wrapper.rs` around
lines 289 - 300, Update gen_client_cfg to return SResult<noq::ClientConfig> and
replace the load_native_certs().expect(...) and pem_file_iter panic path with
propagated errors. Adjust the client new() call site to handle the returned
result so invalid system roots or a missing certificate file surface as
initialization errors instead of aborting.
Comment on lines
+48
to
+63
| pub async fn get_conn(&self) -> Result<SunnyQuicConn, SError> { | ||
| let addr = self | ||
| .config | ||
| .addr | ||
| .to_socket_addrs() | ||
| .unwrap_or_else(|_| panic!("resolve quic addr faile: {}", self.config.addr)) | ||
| .next() | ||
| .unwrap_or_else(|| panic!("resolve quic addr faile: {}", self.config.addr)); | ||
| let end = self | ||
| .quic_end | ||
| .get_or_init(|| async { | ||
| self.init_endpoint() | ||
| .await | ||
| .expect("error during initialize quic endpoint") | ||
| }) | ||
| .await; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Replace panics with error propagation in get_conn.
get_conn returns Result<SunnyQuicConn, SError> but panics on DNS resolution failure (lines 52-55) and endpoint initialization failure (line 61). As per coding guidelines, **/*.rs files should use comprehensive error types with thiserror for error handling. Use SError::DomainResolveFailed and propagate the init error instead.
🔒 Proposed fix
pub async fn get_conn(&self) -> Result<SunnyQuicConn, SError> {
let addr = self
.config
.addr
.to_socket_addrs()
- .unwrap_or_else(|_| panic!("resolve quic addr faile: {}", self.config.addr))
+ .map_err(|_| SError::DomainResolveFailed(self.config.addr.clone()))?
.next()
- .unwrap_or_else(|| panic!("resolve quic addr faile: {}", self.config.addr));
+ .ok_or_else(|| SError::DomainResolveFailed(self.config.addr.clone()))?;
let end = self
.quic_end
- .get_or_init(|| async {
- self.init_endpoint()
- .await
- .expect("error during initialize quic endpoint")
- })
+ .get_or_try_init(|| async { self.init_endpoint().await })
.await?;Note: get_or_try_init requires the try feature on tokio::sync::OnceCell. If unavailable, store the endpoint in an ArcSwapOption and initialize manually with error propagation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub async fn get_conn(&self) -> Result<SunnyQuicConn, SError> { | |
| let addr = self | |
| .config | |
| .addr | |
| .to_socket_addrs() | |
| .unwrap_or_else(|_| panic!("resolve quic addr faile: {}", self.config.addr)) | |
| .next() | |
| .unwrap_or_else(|| panic!("resolve quic addr faile: {}", self.config.addr)); | |
| let end = self | |
| .quic_end | |
| .get_or_init(|| async { | |
| self.init_endpoint() | |
| .await | |
| .expect("error during initialize quic endpoint") | |
| }) | |
| .await; | |
| pub async fn get_conn(&self) -> Result<SunnyQuicConn, SError> { | |
| let addr = self | |
| .config | |
| .addr | |
| .to_socket_addrs() | |
| .map_err(|_| SError::DomainResolveFailed(self.config.addr.clone()))? | |
| .next() | |
| .ok_or_else(|| SError::DomainResolveFailed(self.config.addr.clone()))?; | |
| let end = self | |
| .quic_end | |
| .get_or_try_init(|| async { self.init_endpoint().await }) | |
| .await?; |
🧰 Tools
🪛 GitHub Actions: CI / Build _ prepare
[error] 53-53: typos: faile should be failed, fail (panic message: "resolve quic addr faile").
[error] 55-55: typos: faile should be failed, fail (panic message: "resolve quic addr faile").
🤖 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 `@vendor/shadowquic/shadowquic/src/sunnyquic/outbound.rs` around lines 48 - 63,
Update get_conn to propagate failures through Result instead of panicking:
replace the addr DNS resolution unwrap_or_else/unwrap_or_else chain with
SError::DomainResolveFailed, preserving the configured address in the error, and
propagate init_endpoint errors from quic_end using fallible OnceCell
initialization (get_or_try_init if supported). Keep successful endpoint and
connection creation behavior unchanged.
Source: Coding guidelines
Comment on lines
+53
to
+55
| .unwrap_or_else(|_| panic!("resolve quic addr faile: {}", self.config.addr)) | ||
| .next() | ||
| .unwrap_or_else(|| panic!("resolve quic addr faile: {}", self.config.addr)); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix typos flagged by CI.
- Lines 53, 55: "faile" → "failed"
- Line 101: "connectin" → "connection"
🔧 Proposed fixes
- .unwrap_or_else(|_| panic!("resolve quic addr faile: {}", self.config.addr))
+ .unwrap_or_else(|_| panic!("resolve quic addr failed: {}", self.config.addr))- .unwrap_or_else(|| panic!("resolve quic addr faile: {}", self.config.addr));
+ .unwrap_or_else(|| panic!("resolve quic addr failed: {}", self.config.addr));- // Creating new connectin
+ // Creating new connectionAlso applies to: 101-101
🧰 Tools
🪛 GitHub Actions: CI / Build _ prepare
[error] 53-53: typos: faile should be failed, fail (panic message: "resolve quic addr faile").
[error] 55-55: typos: faile should be failed, fail (panic message: "resolve quic addr faile").
🤖 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 `@vendor/shadowquic/shadowquic/src/sunnyquic/outbound.rs` around lines 53 - 55,
Correct the panic message typos in the outbound address resolution and
connection handling code: update both occurrences of “faile” in the resolve
error messages near the address-resolution chain to “failed,” and change
“connectin” to “connection” in the message near the connection logic.
Source: Pipeline failures
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
clash-lib/src/proxy/sunnyquic/mod.rs (1)
142-145: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winListener terminates on
update_configfailure.The
?operator at line 145 causeslisten_tcpto returnErrifserver.update_config()fails, permanently killing the listener. A failed user-list update should not bring down the entire inbound. Log the error and continue with the existing config instead.🔧 Proposed fix: log and continue on update_config failure
config.users = to_auth_users(&users); - server - .update_config(&config) - .await - .map_err(io::Error::other)?; + if let Err(error) = server.update_config(&config).await { + warn!( + "sunnyquic inbound {}: failed to update config: {}, keeping existing config", + self.addr, error + ); + continue; + } info!(🤖 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 `@clash-lib/src/proxy/sunnyquic/mod.rs` around lines 142 - 145, Update the server.update_config call in listen_tcp to handle errors locally instead of propagating them with ?, log the failure, and continue serving with the existing configuration.
♻️ Duplicate comments (1)
clash-lib/src/proxy/sunnyquic/mod.rs (1)
126-126: 🩺 Stability & Availability | 🟠 MajorPreserve the client address in dispatched sessions.
This path still falls back to
unspecified_source(self.addr), soSession::sourceis recorded as0.0.0.0:0/[::]:0and downstream observability/policy loses the real peer address. Thread the QUIC connection's remote address throughProxyRequestand use it here.🤖 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 `@clash-lib/src/proxy/sunnyquic/mod.rs` at line 126, Update the dispatched-session path around unspecified_source and ProxyRequest to carry the QUIC connection’s remote address through ProxyRequest, then use that address when constructing Session::source instead of unspecified_source(self.addr). Preserve the existing fallback only where no remote address is available.
🤖 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.
Outside diff comments:
In `@clash-lib/src/proxy/sunnyquic/mod.rs`:
- Around line 142-145: Update the server.update_config call in listen_tcp to
handle errors locally instead of propagating them with ?, log the failure, and
continue serving with the existing configuration.
---
Duplicate comments:
In `@clash-lib/src/proxy/sunnyquic/mod.rs`:
- Line 126: Update the dispatched-session path around unspecified_source and
ProxyRequest to carry the QUIC connection’s remote address through ProxyRequest,
then use that address when constructing Session::source instead of
unspecified_source(self.addr). Preserve the existing fallback only where no
remote address is available.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 142496f7-37cf-4be6-90ef-9e6063809deb
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Cargo.tomlclash-lib/src/proxy/sunnyquic/mod.rssunnyquic-inbound-brutal.yamlsunnyquic-inbound.yaml
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.
What does this PR do?
Type
Checklist
这是一个gpt-5.6-sol模型 写的mr并手动提交的代码
› 请给出入站配置文件样例
其中拥堵控制算法除了可以设置bbr之外也可以设置brutal bandwidth 入站设置服务端下行速度 客户端应设置上行速度 outbound暂时代码未提交 可以使用官方shadowquic客户端
注意：
仓库中也已加入样例：clash-rs/clash-bin/tests/data/config/full.yaml:322。
Summary by CodeRabbit
New Features
Bug Fixes
Chores