Fix IPv4 excludes and SOCKS5 UDP DNS handling - #1
Conversation
|
There was a problem hiding this comment.
Short version: I checked all three issues, they are real, and the fixes are heading the right way, so I am going to merge this.
There is a concurrency problem in the UDP part. I will go into that separately below, along with why I plan to merge first and handle it myself in a follow-up.
Going through them one by one
-
CIDR excludes
This one is real.
default_exclude_cidrsand the per-ruledst_filterwere only ever parsed, never actually used. I designed the feature and then never wired it into the forwarding path, which was my oversight. On the TCP side, doing the check in the SOCKET layer connect event and skipping the PortTracker write on a match is exactly where it belongs, and matches how I would have done it. -
SOCKS5 UDP data socket bound to loopback
Also real. It never showed up because every SOCKS5 proxy I tested against ran on the same machine. With a remote proxy, UDP obviously cannot reach a socket bound to loopback. Binding to the wildcard address is the right direction.
-
DNS restore
This is the one that hurts users most. On a DHCP machine, after Clew exits the adapter's DNS ends up written as a static configuration (holding whatever DHCP handed out when Clew started), and from then on it no longer follows DHCP. Using the registry
NameServervalue to tell whether DNS was explicitly configured, and writing an empty string to put an automatic interface back on DHCP, is the right approach. Adding theautomaticfield to the state file while keeping old state files on their previous behavior is a nice touch on the compatibility side.I ran an A/B check locally. With the adapter switched to automatic DNS, enabling and then disabling the DNS proxy on main logs
SetInterfaceDnsSettings failed: 87(ERROR_INVALID_PARAMETER); the adapter'sNameServerstays at127.0.0.2and does not come back even after Clew exits, which leaves the machine with no DNS at all. On your branch the same sequence restores automatic mode correctly. The empty-string path works. -
Tests
Good both in quantity and quality, especially the local SOCKS5 UDP peer stub, which will stay useful for a long time. Exclude ranges, rule priority, how excludes behave for manual hijacks versus auto rules, DNS restore modes, process tree inheritance -- these test observable behavior rather than internals, so I can keep them as regression tests through the refactor described below.
I ran them here as well: 52/52 component tests, 21/21 e2e, all passing.
The concurrency problem with the UDP snapshot
UdpPortTracker is a lock-free slot array. The only writes are put() and clear() on the strand; the reads come from the two NETWORK worker threads. It relies on release/acquire on active so that a reader seeing active == true also sees a fully written entry. The thing to note is that release/acquire only orders visibility at publication, it does not provide mutual exclusion. When put() overwrites the same slot, a reader can still be in the middle of reading that entry. While the entry holds only POD / trivially copyable data, that is a deliberate trade-off: a torn read at worst means one packet uses the wrong group_id, which is bounded, and the next packet is fine. Putting a shared_ptr in the entry breaks that premise. get() copies the whole entry by value, which includes copy-constructing the shared_ptr, while put() may be overwriting that same entry by assignment. That is a concurrent read and write of the same shared_ptr object, which is undefined behavior.
It is not only a data race. The slot is the only owner of the policy, so overwriting the slot can destroy the old control block immediately. If a reader has just picked up the internal pointer but has not incremented the refcount yet, it ends up touching freed memory. This window does not depend on port reuse, and it is not rare in practice.
The SOCKET layer calls put() once for the BIND event and once for the CONNECT event, the second time to fill in RemoteAddr. As a side note, nothing on the UDP path currently reads those two address fields. By the time the second put() happens, active is already true and the application may already be sending, so reads and writes overlap easily. Essentially every connected UDP socket passes through this window while it is being set up.
On top of that, clear() only sets active to false without clearing the entry, so the old policy object stays held by the slot and is not released in time.
Why I am merging as-is
The minimal fix would be to move the shared_ptr out of the entry and keep a separate std::atomic<std::shared_ptr<...>> in the slot. Looking one step further though, the content of this policy depends only on the global config and the rules, and has nothing to do with the port. Keeping a per-port snapshot means copying the same data over and over.
So I am going to turn it into a single shared immutable policy table:
- the strand rebuilds the whole table whenever the config changes;
- the new table is published atomically together with a version counter;
- workers keep a
thread_localcache and only compare the version once per packet; UdpPortTrackerslots carry a rule id (the TCP side does not need one: TCP already makes its decision in the connect event);- the entry goes back to being trivially copyable, with a
static_assertso nobody puts a member with a non-trivial copy into it again; the TCP side gets the same check.
That change replaces the snapshot mechanism outright, so there is no point in asking you to write an intermediate version that would just be deleted afterwards.
I will merge this PR first and do that work in a follow-up, finished before the next release. Only about thirty lines on the UDP data path get replaced; the exclude rule logic you wrote, the TCP path, the config wiring, the DNS fix, the UDP session fix and all of the tests stay.
There is one related item from the wildcard bind that I will handle at the same time: once the socket is bound to 0.0.0.0, anyone can send to that port. The downstream receive path does not check the sender today, so datagrams where from != relay endpoint need to be dropped.
Thanks again for the contribution.
This is the first external PR Clew has received, and having it arrive at this quality is not something I expected. It made my day.
|
Thank you for the thorough review, local validation, and detailed explanation of the UDP concurrency issue. The shared immutable policy table is clearly a better fit than keeping an owning pointer in each lock-free tracker slot. I also agree with validating the sender endpoint after binding the UDP socket to the wildcard address. I saw that the follow-up fix and v0.9.5 have already landed. I’m glad the behavior-focused tests remain useful through the refactor. For transparency, I’m primarily a Clew user rather than a C++ maintainer, and I used Codex extensively to help investigate the failures, implement the changes, and build the regression tests. I provided the real-world reproductions, logs, requirements, and Windows validation, and reviewed the resulting behavior before submitting the PR. Thank you again for merging the contribution and for the kind feedback. |



Summary
Fix three related gaps in the Windows proxy path:
Changes
NameServerregistry value and reset automatic DNS with a valid emptyNameServerstring.Validation
Risks and limitations