Conversation
|
Important Review skippedToo many files! This PR contains 193 files, which is 43 over the limit of 150. 📒 Files selected for processing (193)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86fa919e9a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+411
to
+412
| cancellationToken.ThrowIfCancellationRequested(); | ||
| ThrowIfDisposed(); |
There was a problem hiding this comment.
CreateRuntimeAsync can return from ZeroTierSocketRuntimeBootstrapper.CreateAsync with a live ZeroTierDataplaneRuntime and then throw at the immediate cancellation/disposed checks, which means the newly created runtime is never assigned to _runtime and never disposed. In the DisposeAsync race (shutdown token canceled while runtime creation is finishing), this leaks active background loops and UDP sockets because DisposeAsync only disposes the _runtime field.
Useful? React with 👍 / 👎.
Comment on lines
+341
to
+342
| var (packetId, verb) = TryGetPacketIdAndVerb(packet, out var parsed) ? parsed : default; | ||
| if (verb != ZeroTierVerb.QosMeasurement) |
There was a problem hiding this comment.
This QoS gate parses the verb from packet after it has already been armored (the outbound builders encrypt starting at IndexVerb), so the extracted verb is ciphertext-derived rather than the real protocol verb. That makes the verb != QosMeasurement condition nondeterministic for normal traffic and intermittently skips _peerQos.RecordOutgoingPacket, which drops valid QoS samples and degrades multipath path selection quality.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
ZTSharp/Sockets/OverlayTcpIncomingBuffer.cs (1)
63-83:⚠️ Potential issue | 🟠 MajorFault can be lost as EOF in
ReadAsyncunder race.After the initial fault check at Line 63, a fault set concurrently can still hit the
Line 79_remoteClosedpath and return0instead of throwingIOException. Re-check fault state before returning EOF in-loop.🔧 Proposed fix
if (_currentSegment.Length == 0 || _currentSegmentOffset >= _currentSegment.Length) { while (true) { if (_incoming.Reader.TryRead(out var segment)) { _currentSegment = segment; _currentSegmentOffset = 0; break; } + if (_fault is not null) + { + throw _fault; + } + if (_remoteClosed) { return 0; } try🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ZTSharp/Sockets/OverlayTcpIncomingBuffer.cs` around lines 63 - 83, In ReadAsync (class OverlayTcpIncomingBuffer) a concurrent _fault can be set after the initial check and before the loop returns 0; update the loop so that just before taking the _remoteClosed -> return 0 path you re-check _fault and throw it if non-null (i.e. re-evaluate _fault inside the while loop before returning EOF), referencing the existing _fault field and the _remoteClosed check around the _incoming.Reader.TryRead handling to ensure the IOException is not lost.ZTSharp/ZeroTier/Internal/ZeroTierDataplaneRouteRegistry.cs (1)
327-358:⚠️ Potential issue | 🟠 MajorValidate
destinationIpfamily againstaddressFamilybefore lookup.Without an upfront consistency check, mismatched inputs can produce incorrect dispatch behavior or runtime failure paths.
Suggested fix
public bool TryGetTcpSynHandler( AddressFamily addressFamily, IPAddress destinationIp, ushort localPort, out Func<NodeId, ReadOnlyMemory<byte>, CancellationToken, Task> handler) { + ArgumentNullException.ThrowIfNull(destinationIp); + if (destinationIp.AddressFamily != addressFamily) + { + throw new ArgumentException( + "Destination IP address family must match the provided address family.", + nameof(destinationIp)); + } + if (addressFamily == AddressFamily.InterNetwork) { if (_tcpListenersV4.TryGetValue(localPort, out var registrations) && registrations.TryGet(destinationIp, out var existing))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ZTSharp/ZeroTier/Internal/ZeroTierDataplaneRouteRegistry.cs` around lines 327 - 358, Ensure TryGetTcpSynHandler validates that the provided destinationIp matches the requested addressFamily before looking up listeners: at the top of TryGetTcpSynHandler check destinationIp.AddressFamily against the addressFamily parameter and if they differ either normalize mapped addresses (e.g., call destinationIp.MapToIPv4() when addressFamily==AddressFamily.InterNetwork and destinationIp.IsIPv4MappedToIPv6) or throw an ArgumentException for invalid combinations (using nameof(destinationIp)); then proceed to use _tcpListenersV4/_tcpListenersV6 and the existing lookup logic so mismatched inputs cannot produce incorrect dispatch behavior.ZTSharp/Transport/Internal/OsUdpPeerRegistry.cs (1)
78-99:⚠️ Potential issue | 🟠 MajorTTL/cap eviction does not apply to
_networkPeers, so stale peers can accumulate.
SweepDirectorytrimss_networkDirectory, but both registration paths only upsert into_networkPeersand never remove entries that were expired/trimmed from the directory. That meansTryGetPeerscan retain dead endpoints and keep growing under peer churn, which undermines the bounded-directory hardening.Also applies to: 108-115, 145-213
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ZTSharp/Transport/Internal/OsUdpPeerRegistry.cs` around lines 78 - 99, The _networkPeers map isn't being pruned when s_networkDirectory entries are evicted: after calling SweepDirectory(nowTicks) and obtaining discoveredPeers, update localPeers (the ConcurrentDictionary from _networkPeers for the networkId) so it only contains keys present in discoveredPeers — add/overwrite entries for discoveredPeers as you already do and remove any keys in localPeers that are not in discoveredPeers (use TryRemove to be thread-safe). Apply the same removal logic to the other registration paths referenced (where discoveredPeers/localPeers are manipulated around lines 108-115 and 145-213) so that _networkPeers never retains endpoints that were trimmed from s_networkDirectory; use the same symbols: s_networkDirectory, discoveredPeers, localPeers, _networkPeers, SweepDirectory, TryRemove, localNodeId.
🟡 Minor comments (12)
ZTSharp/ZeroTier/Internal/ZeroTierDataplaneRouteRegistry.cs-246-259 (1)
246-259:⚠️ Potential issue | 🟡 MinorAdd a null guard for
onSynin listener registration.This method now accepts externally supplied callback state; it should validate
onSynalongsidelocalAddress.Suggested fix
public bool TryRegisterTcpListener( IPAddress localAddress, ushort localPort, Func<NodeId, ReadOnlyMemory<byte>, CancellationToken, Task> onSyn) { ArgumentNullException.ThrowIfNull(localAddress); + ArgumentNullException.ThrowIfNull(onSyn); return localAddress.AddressFamily switch🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ZTSharp/ZeroTier/Internal/ZeroTierDataplaneRouteRegistry.cs` around lines 246 - 259, The TryRegisterTcpListener method lacks validation for the onSyn callback; add a null guard alongside the existing localAddress check by calling ArgumentNullException.ThrowIfNull(onSyn) at the start of TryRegisterTcpListener (before the AddressFamily switch) so the method throws immediately if the onSyn delegate is null; keep the rest of the logic (the _tcpListenersV4/_tcpListenersV6 GetOrAdd and TryAdd calls) unchanged.ZTSharp/EventLoop.cs-34-34 (1)
34-34:⚠️ Potential issue | 🟡 MinorPotential visibility issue when reading
_faultwithout synchronization.The
_faultfield is written under_gatelock (line 158) but read without synchronization inThrowIfDisposed()(line 290). This could cause visibility issues on weakly-ordered memory architectures where the read might not observe the write.Consider using
Volatile.Readfor the check:Proposed fix
private void ThrowIfDisposed() { - if (_fault is not null) + var fault = Volatile.Read(ref _fault); + if (fault is not null) { - throw new InvalidOperationException("Event loop is faulted.", _fault); + throw new InvalidOperationException("Event loop is faulted.", fault); } ObjectDisposedException.ThrowIf(_disposed, this); }Also applies to: 290-293
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ZTSharp/EventLoop.cs` at line 34, The _fault field is written while holding the _gate lock but read unsynchronized in ThrowIfDisposed(), risking visibility on weak memory models; change the read in ThrowIfDisposed() to use a volatile read (e.g., use System.Threading.Volatile.Read(ref _fault)) and base the null check / throw on that value, so the thread observes the latest exception written under the _gate lock; keep the write under _gate unchanged and apply the same Volatile.Read pattern to any other unsynchronized reads of _fault (lines referenced around ThrowIfDisposed and the _fault declaration).ZTSharp/ZeroTier/Internal/ManagedIpToNodeIdCache.cs-76-94 (1)
76-94:⚠️ Potential issue | 🟡 MinorEviction queue may contain duplicate keys, causing premature eviction of refreshed entries.
When an entry is updated (lines 89-90),
EnqueueForEvictionis called again, adding a duplicate key to the queue. Later,EnforceCapacitymay dequeue the older occurrence and remove a still-valid, recently-refreshed entry.Consider either:
- Tracking whether the key is already in the queue (e.g., with a
HashSet)- Checking entry staleness in
EnforceCapacitybefore removal- Only enqueueing on actual insertion (not update)
🔧 Suggested fix: Only enqueue on new insertions
_entries.AddOrUpdate( managedIp, _ => { EnqueueForEviction(managedIp); return new Entry(nodeId, expiresAt, isAuthoritative); }, (_, existing) => { if (IsExpired(existing)) { - EnqueueForEviction(managedIp); return new Entry(nodeId, expiresAt, isAuthoritative); } if (!isAuthoritative && existing.IsAuthoritative) { return existing; } - EnqueueForEviction(managedIp); return new Entry(nodeId, expiresAt, isAuthoritative); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ZTSharp/ZeroTier/Internal/ManagedIpToNodeIdCache.cs` around lines 76 - 94, The eviction queue can get duplicate keys because EnqueueForEviction is called on every AddOrUpdate; change the update logic in ManagedIpToNodeIdCache so EnqueueForEviction is only called for true insertions or when replacing an expired entry: inside the AddOrUpdate delegate (the lambda that receives existing), call EnqueueForEviction only when existing is null (new insertion) or IsExpired(existing) is true, and do not enqueue for harmless refreshes or when an authoritative existing entry should be kept; this avoids duplicate keys in the queue and prevents EnforceCapacity from evicting recently-refreshed entries.ZTSharp.Tests/AtomicFileTests.cs-10-23 (1)
10-23:⚠️ Potential issue | 🟡 MinorMissing cleanup of temporary test directory.
The test creates a temporary directory at
rootbut never cleans it up after the test completes. This could leave orphaned directories on disk over repeated test runs.🧹 Proposed fix to add cleanup
[Fact] public async Task WriteAllBytesAsync_Throws_WhenAtomicMoveNeverSucceeds() { var root = TestTempPaths.CreateGuidSuffixed("zt-atomic-file-"); Directory.CreateDirectory(root); - - var destination = Path.Combine(root, "dest"); - Directory.CreateDirectory(destination); - - var ex = await Assert.ThrowsAsync<IOException>(async () => + try { - await AtomicFile.WriteAllBytesAsync(destination, new byte[] { 1, 2, 3 }, CancellationToken.None); - }); + var destination = Path.Combine(root, "dest"); + Directory.CreateDirectory(destination); - Assert.Contains("Atomic replace failed", ex.Message, StringComparison.OrdinalIgnoreCase); - Assert.True(Directory.Exists(destination)); + var ex = await Assert.ThrowsAsync<IOException>(async () => + { + await AtomicFile.WriteAllBytesAsync(destination, new byte[] { 1, 2, 3 }, CancellationToken.None); + }); + + Assert.Contains("Atomic replace failed", ex.Message, StringComparison.OrdinalIgnoreCase); + Assert.True(Directory.Exists(destination)); + } + finally + { + Directory.Delete(root, recursive: true); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ZTSharp.Tests/AtomicFileTests.cs` around lines 10 - 23, The test creates a temp directory stored in variable root via TestTempPaths.CreateGuidSuffixed and never deletes it; wrap the test body in a try/finally (or add teardown) and ensure Directory.Delete(root, recursive: true) is called in the finally block so the temporary directory is removed regardless of test outcome; keep existing assertions and ensure you reference root when deleting to clean up after AtomicFile.WriteAllBytesAsync usage.docs/ZEROTIER_STACK.md-65-67 (1)
65-67:⚠️ Potential issue | 🟡 MinorDocument CLI multipath validation constraints to match runtime behavior.
The flag list is helpful, but it should also state that UDP socket count must be
1..8, and--mp-udp-portscount must match--mp-udp-socketswith each port in0..65535.Suggested doc patch
To enable experimental multipath/bonding for the managed stack, use the CLI flags: `--multipath`, `--mp-bond`, `--mp-udp-sockets`, `--mp-udp-ports`, `--mp-warmup-root`. +Constraints: `--mp-udp-sockets` must be in `1..8`. If `--mp-udp-ports` is set, it must include exactly that many entries, each in `0..65535` (`0` = ephemeral).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ZEROTIER_STACK.md` around lines 65 - 67, Update the ZEROTIER_STACK.md multipath CLI section to document the runtime validation rules: state that --mp-udp-sockets must be an integer in the range 1..8, --mp-udp-ports must provide the same number of ports as --mp-udp-sockets, and each port value must be in the range 0..65535; list these constraints alongside the existing flags (--multipath, --mp-bond, --mp-udp-sockets, --mp-udp-ports, --mp-warmup-root) and add a brief example or note showing matching counts for --mp-udp-sockets and --mp-udp-ports.ZTSharp.Tests/TunnelAndHttpTests.cs-309-353 (1)
309-353:⚠️ Potential issue | 🟡 MinorSynchronize access to
List<Task>to prevent race conditions.The
connectionTaskslist is mutated on line 352 from a thread pool thread (insideTask.Run) while being enumerated on line 405 from the test thread.List<T>is not thread-safe for concurrent mutations and enumerations, which can cause items to be skipped or the test to intermittently fail.🧩 Proposed fix
- var connectionTasks = new List<Task>(capacity: 3); + var connectionTasks = new List<Task>(capacity: 3); + var connectionTasksLock = new object(); @@ - connectionTasks.Add(HandleConnectionAsync(tcp, releaseTcs.Task, cts.Token)); + lock (connectionTasksLock) + { + connectionTasks.Add(HandleConnectionAsync(tcp, releaseTcs.Task, cts.Token)); + } @@ - await Task.WhenAll(connectionTasks.Concat(new[] { acceptTask, forwarderTask })).WaitAsync(TimeSpan.FromSeconds(2)); + Task[] connectionSnapshot; + lock (connectionTasksLock) + { + connectionSnapshot = connectionTasks.ToArray(); + } + + await Task.WhenAll(connectionSnapshot.Concat(new[] { acceptTask, forwarderTask })) + .WaitAsync(TimeSpan.FromSeconds(2));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ZTSharp.Tests/TunnelAndHttpTests.cs` around lines 309 - 353, The test mutates connectionTasks concurrently (added in acceptTask) while being enumerated later, causing race conditions; replace the non-thread-safe List<Task> connectionTasks with a thread-safe collection (e.g., System.Collections.Concurrent.ConcurrentBag<Task>) or ensure synchronized access: change the declaration of connectionTasks to ConcurrentBag<Task> and keep using connectionTasks.Add(...) inside the acceptTask loop, then await or enumerate via Task.WhenAll(connectionTasks) or connectionTasks.ToArray() where the test waits on them; alternatively, if you prefer locking, wrap all mutations and enumerations of connectionTasks with the same lock object referenced from HandleConnectionAsync, acceptTask, and the test assertion code.samples/ZTSharp.Cli/Commands/JoinCommand.cs-126-133 (1)
126-133:⚠️ Potential issue | 🟡 MinorValidate multipath
LocalUdpPortslength vsUdpSocketCountbefore passing options.At Line [130], defaulting
UdpSocketCountto 1 can create inconsistent configuration when a multi-port--mp-udp-portslist is supplied.🔧 Proposed fix
+ var resolvedUdpSocketCount = mpUdpSockets ?? mpUdpPorts?.Count ?? 1; + if (mpUdpPorts is not null && + mpUdpSockets is not null && + mpUdpPorts.Count != mpUdpSockets.Value) + { + throw new InvalidOperationException("Invalid multipath config: --mp-udp-ports count must match --mp-udp-sockets."); + } + var multipath = new ZeroTierMultipathOptions { Enabled = mpEnabled, BondPolicy = mpBondPolicy, - UdpSocketCount = mpUdpSockets ?? 1, + UdpSocketCount = resolvedUdpSocketCount, LocalUdpPorts = mpUdpPorts, WarmupDuplicateToRoot = mpWarmupRoot };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@samples/ZTSharp.Cli/Commands/JoinCommand.cs` around lines 126 - 133, The multipath options may become inconsistent because UdpSocketCount is defaulted to 1 while LocalUdpPorts can be a multi-port list; update the construction of ZeroTierMultipathOptions to validate and reconcile UdpSocketCount and LocalUdpPorts: if mpUdpPorts is provided, set or adjust UdpSocketCount to match mpUdpPorts.Count (or ensure UdpSocketCount <= mpUdpPorts.Count) and surface a clear error or fallback if they conflict; perform this check before creating the ZeroTierMultipathOptions instance and then populate Enabled, BondPolicy, UdpSocketCount, LocalUdpPorts, and WarmupDuplicateToRoot with the reconciled values.samples/ZTSharp.Cli/Commands/ExposeCommand.cs-149-156 (1)
149-156:⚠️ Potential issue | 🟡 MinorValidate multipath
LocalUdpPortslength againstUdpSocketCount.At Line [153],
UdpSocketCount = mpUdpSockets ?? 1can conflict withLocalUdpPortswhen--mp-udp-portsis provided without--mp-udp-sockets(or with a mismatched count). That creates an invalid options object shape.🔧 Proposed fix
+ var resolvedUdpSocketCount = mpUdpSockets ?? mpUdpPorts?.Count ?? 1; + if (mpUdpPorts is not null && + mpUdpSockets is not null && + mpUdpPorts.Count != mpUdpSockets.Value) + { + throw new InvalidOperationException("Invalid multipath config: --mp-udp-ports count must match --mp-udp-sockets."); + } + var multipath = new ZeroTierMultipathOptions { Enabled = mpEnabled, BondPolicy = mpBondPolicy, - UdpSocketCount = mpUdpSockets ?? 1, + UdpSocketCount = resolvedUdpSocketCount, LocalUdpPorts = mpUdpPorts, WarmupDuplicateToRoot = mpWarmupRoot };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@samples/ZTSharp.Cli/Commands/ExposeCommand.cs` around lines 149 - 156, When constructing the ZeroTierMultipathOptions (variable multipath) ensure LocalUdpPorts and UdpSocketCount are consistent: if mpUdpPorts is provided and mpUdpSockets is null, set UdpSocketCount = mpUdpPorts.Length; if both mpUdpPorts and mpUdpSockets are provided validate mpUdpPorts.Length == mpUdpSockets and fail early (throw or print error/exit) on mismatch; otherwise keep the existing default of 1. Update the code around ZeroTierMultipathOptions (multipath, UdpSocketCount, LocalUdpPorts, mpUdpSockets, mpUdpPorts) to implement these checks and behaviors.ZTSharp.Tests/ZeroTierSizeCapHardeningTests.cs-55-55 (1)
55-55:⚠️ Potential issue | 🟡 MinorDispose
ZeroTierDataplanePeerSecurityin this test.Line 55 creates an
IDisposableand never disposes it.💡 Proposed fix
- var peerSecurity = new ZeroTierDataplanePeerSecurity(udp, rootClient, localIdentity); + using var peerSecurity = new ZeroTierDataplanePeerSecurity(udp, rootClient, localIdentity);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ZTSharp.Tests/ZeroTierSizeCapHardeningTests.cs` at line 55, The test creates an IDisposable ZeroTierDataplanePeerSecurity instance (constructed as new ZeroTierDataplanePeerSecurity(udp, rootClient, localIdentity)) and never disposes it; update the test to dispose that instance (e.g., wrap the construction in a using statement or ensure disposal in a finally block) so ZeroTierDataplanePeerSecurity is properly disposed at the end of the test.ZTSharp/ZeroTier/Internal/ZeroTierExternalSurfaceAddressTracker.cs-24-31 (1)
24-31:⚠️ Potential issue | 🟡 MinorStore and return defensive
IPEndPointcopies.Keeping shared mutable
IPEndPointinstances allows caller-side mutation to leak back into tracker state.Proposed fix
- _entries[key] = new Entry(surfaceAddress, now); + var stored = new IPEndPoint(surfaceAddress.Address, surfaceAddress.Port); + _entries[key] = new Entry(stored, now); @@ - list.Add(entry.SurfaceAddress); + list.Add(new IPEndPoint(entry.SurfaceAddress.Address, entry.SurfaceAddress.Port));Also applies to: 40-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ZTSharp/ZeroTier/Internal/ZeroTierExternalSurfaceAddressTracker.cs` around lines 24 - 31, In ZeroTierExternalSurfaceAddressTracker, avoid storing or returning references to caller-owned mutable IPEndPoint objects: in Observe(NodeId reportingPeerNodeId, int localSocketId, IPEndPoint surfaceAddress) create and store a defensive copy of surfaceAddress (e.g. clone) when constructing new Entry(_entries[key]) and when any lookup/peek/get method (the methods around lines 40-45 that return stored endpoints) return an IPEndPoint, return a defensive copy instead of the stored instance; update Entry (and any accessor) to hold the copied IPEndPoint so external mutation cannot affect tracker state and ensure CleanupIfNeeded and _entries usage stays the same.samples/ZTSharp.Cli/Commands/ListenCommand.cs-102-109 (1)
102-109:⚠️ Potential issue | 🟡 MinorValidate/infer UDP socket count when
--mp-udp-portsis provided.With
--mp-udp-portsand no--mp-udp-sockets, this defaults toUdpSocketCount = 1, which can conflict and fail later in transport bootstrapper validation.Proposed fix
- var multipath = new ZeroTierMultipathOptions + var udpSocketCount = mpUdpSockets ?? mpUdpPorts?.Count ?? 1; + if (mpUdpPorts is not null && mpUdpPorts.Count != udpSocketCount) + { + throw new InvalidOperationException("--mp-udp-ports count must match --mp-udp-sockets."); + } + + var multipath = new ZeroTierMultipathOptions { Enabled = mpEnabled, BondPolicy = mpBondPolicy, - UdpSocketCount = mpUdpSockets ?? 1, + UdpSocketCount = udpSocketCount, LocalUdpPorts = mpUdpPorts, WarmupDuplicateToRoot = mpWarmupRoot };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@samples/ZTSharp.Cli/Commands/ListenCommand.cs` around lines 102 - 109, The multipath UdpSocketCount is currently set to mpUdpSockets ?? 1 which ignores mpUdpPorts and can cause validation failures; update the ZeroTierMultipathOptions construction (ZeroTierMultipathOptions, UdpSocketCount, LocalUdpPorts) so that when mpUdpSockets is null and mpUdpPorts is provided you infer UdpSocketCount from the number of provided UDP ports (e.g. count the entries in mpUdpPorts) and fall back to 1 only if neither is present; ensure the logic uses mpUdpSockets if set, otherwise derives the count from mpUdpPorts, then assigns that value to UdpSocketCount.ZTSharp/ZeroTier/Internal/ZeroTierPeerEchoManager.cs-162-177 (1)
162-177:⚠️ Potential issue | 🟡 MinorValidate Echo-OK path/payload before removing pending state.
_pendingByPacketIdis removed before path and payload checks. A non-matching/malformed OK can drop the valid pending probe and suppress later legitimate RTT updates.Proposed fix
- if (!_pendingByPacketId.TryRemove(inRePacketId, out var pending)) + if (!_pendingByPacketId.TryGetValue(inRePacketId, out var pending)) { return; } @@ if (okPayloadTail.Length < 8) { return; } @@ - _lastRttMsByPath[pending.PathKey] = (int)rtt; + if (!_pendingByPacketId.TryRemove(inRePacketId, out _)) + { + return; + } + + _lastRttMsByPath[pending.PathKey] = (int)rtt;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ZTSharp/ZeroTier/Internal/ZeroTierPeerEchoManager.cs` around lines 162 - 177, You remove the pending entry from _pendingByPacketId before validating that the Echo-OK matches the pending probe; change the logic to first retrieve and validate the pending (use _pendingByPacketId.TryGetValue(inRePacketId, out var pending)), check pending.PathKey.PeerNodeId, pending.PathKey.Path.LocalSocketId, pending.PathKey.Path.RemoteEndPoint and okPayloadTail.Length >= 8, and only then remove the entry (call _pendingByPacketId.TryRemove(inRePacketId, out _)) so you don't drop a valid pending probe on mismatched or malformed OKs; reference the symbols _pendingByPacketId, inRePacketId, pending, PathKey, PeerNodeId, Path.LocalSocketId, Path.RemoteEndPoint and okPayloadTail when applying this change.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (193)
ZTSharp.Tests/ActiveTaskSetTests.csZTSharp.Tests/AtomicFileTests.csZTSharp.Tests/ChannelWriterConcurrencyTests.csZTSharp.Tests/EventLoopTests.csZTSharp.Tests/FileStateStoreSecurityTests.csZTSharp.Tests/InMemoryNodeTransportCancellationTests.csZTSharp.Tests/Ipv4CodecChecksumTests.csZTSharp.Tests/ManagedIpToNodeIdCacheTests.csZTSharp.Tests/NodeEventHandlerIsolationTests.csZTSharp.Tests/NodeNetworkLeaveOrderingTests.csZTSharp.Tests/NodeTransportTests.csZTSharp.Tests/OsUdpPeerDiscoveryTests.csZTSharp.Tests/OsUdpPeerPersistenceTests.csZTSharp.Tests/OsUdpPeerRegistryBoundsTests.csZTSharp.Tests/OsUdpPeerRegistryCollection.csZTSharp.Tests/OsUdpReceiveLoopSocketExceptionTests.csZTSharp.Tests/OsUdpSendFrameResilienceTests.csZTSharp.Tests/OsUdpSocketFactoryTests.csZTSharp.Tests/OsUdpSpoofingTests.csZTSharp.Tests/OverlayTcpBackgroundTaskSafetyTests.csZTSharp.Tests/OverlayTcpIncomingBufferTests.csZTSharp.Tests/SecretFilePermissionTests.csZTSharp.Tests/StateStoreKeyNormalizationTests.csZTSharp.Tests/TcpCodecEncodeBoundsTests.csZTSharp.Tests/TestUdpEndpoints.csZTSharp.Tests/TunnelAndHttpTests.csZTSharp.Tests/UdpEndpointNormalizationTests.csZTSharp.Tests/UserSpaceTcpDisposeRaceTests.csZTSharp.Tests/UserSpaceTcpFinTests.csZTSharp.Tests/UserSpaceTcpHalfCloseTests.csZTSharp.Tests/UserSpaceTcpHandshakeTests.csZTSharp.Tests/UserSpaceTcpRemoteWindowTests.csZTSharp.Tests/UserSpaceTcpStressTests.csZTSharp.Tests/ZeroTierDataplaneFragmentPolicyTests.csZTSharp.Tests/ZeroTierDataplaneRootClientResolveNodeIdTests.csZTSharp.Tests/ZeroTierDataplaneRuntimeDirectPathTests.csZTSharp.Tests/ZeroTierDataplaneRxLoopTests.csZTSharp.Tests/ZeroTierDataplaneShutdownTests.csZTSharp.Tests/ZeroTierDataplaneTcpChecksumTests.csZTSharp.Tests/ZeroTierDataplaneUdpChecksumTests.csZTSharp.Tests/ZeroTierDirectEndpointManagerPushFlagsTests.csZTSharp.Tests/ZeroTierDirectEndpointSelectionTests.csZTSharp.Tests/ZeroTierExternalSurfaceAddressTrackerTests.csZTSharp.Tests/ZeroTierFlowIdTests.csZTSharp.Tests/ZeroTierHelloClientTests.csZTSharp.Tests/ZeroTierHelloOkParserDecryptedTests.csZTSharp.Tests/ZeroTierHttpMessageHandlerTests.csZTSharp.Tests/ZeroTierIcmpv6ChecksumTests.csZTSharp.Tests/ZeroTierInlineComTests.csZTSharp.Tests/ZeroTierIpv4LinkSenderTests.csZTSharp.Tests/ZeroTierManagedSocketLifecycleTests.csZTSharp.Tests/ZeroTierMulticastGatherClientTests.csZTSharp.Tests/ZeroTierNetworkConfigClientTests.csZTSharp.Tests/ZeroTierNetworkConfigProtocolTests.csZTSharp.Tests/ZeroTierPacketCryptoAesGmacSivTests.csZTSharp.Tests/ZeroTierPeerBondPolicyEngineTests.csZTSharp.Tests/ZeroTierPeerEchoManagerTests.csZTSharp.Tests/ZeroTierPeerPathNegotiationManagerTests.csZTSharp.Tests/ZeroTierPeerPhysicalPathTrackerTests.csZTSharp.Tests/ZeroTierPeerQosManagerTests.csZTSharp.Tests/ZeroTierPeerSecurityNegativeCacheTests.csZTSharp.Tests/ZeroTierRoutedLinkOverflowTests.csZTSharp.Tests/ZeroTierSizeCapHardeningTests.csZTSharp.Tests/ZeroTierSocketAnyScopeSemanticsTests.csZTSharp.Tests/ZeroTierSocketFactoryStateRootTests.csZTSharp.Tests/ZeroTierSocketRuntimeBootstrapperUdpTransportTests.csZTSharp.Tests/ZeroTierStateFileSizeCapTests.csZTSharp.Tests/ZeroTierTcpListenerBacklogTests.csZTSharp.Tests/ZeroTierTimeoutsTests.csZTSharp.Tests/ZeroTierUdpMultiTransportTests.csZTSharp.Tests/ZeroTierUdpSocketDisposeTests.csZTSharp.Tests/ZeroTierUdpTransportTests.csZTSharp.Tests/ZeroTierWhoisClientTests.csZTSharp.Tests/ZeroTierWorldCodecTests.csZTSharp/EventLoop.csZTSharp/FileStateStore.csZTSharp/Http/OverlayHttpMessageHandler.csZTSharp/Http/OwnedOverlayTcpClientStream.csZTSharp/Internal/ActiveTaskSet.csZTSharp/Internal/AtomicFile.csZTSharp/Internal/BoundedFileIO.csZTSharp/Internal/NodeCore.csZTSharp/Internal/NodeEventStream.csZTSharp/Internal/NodeIdentityService.csZTSharp/Internal/NodeLifecycleService.csZTSharp/Internal/NodeNetworkService.csZTSharp/Internal/NodePeerService.csZTSharp/Internal/NodeTransportService.csZTSharp/Internal/SecretFilePermissions.csZTSharp/Sockets/OverlayTcpClient.csZTSharp/Sockets/OverlayTcpIncomingBuffer.csZTSharp/Sockets/OverlayTcpListener.csZTSharp/Sockets/OverlayTcpPortForwarder.csZTSharp/Sockets/ZtUdpClient.csZTSharp/StateStoreKeyNormalization.csZTSharp/StateStoreKeySegmentValidation.csZTSharp/StateStorePrefixNormalization.csZTSharp/Transport/InMemoryNodeTransport.csZTSharp/Transport/Internal/OsUdpPeerDiscoveryProtocol.csZTSharp/Transport/Internal/OsUdpPeerRegistry.csZTSharp/Transport/Internal/OsUdpReceiveLoop.csZTSharp/Transport/Internal/OsUdpSocketFactory.csZTSharp/Transport/Internal/UdpEndpointNormalization.csZTSharp/Transport/OsUdpNodeTransport.csZTSharp/ZeroTier/Http/ZeroTierHttpMessageHandler.csZTSharp/ZeroTier/Internal/ManagedIpToNodeIdCache.csZTSharp/ZeroTier/Internal/ZeroTierDataplaneIcmpv6Handler.csZTSharp/ZeroTier/Internal/ZeroTierDataplaneIpHandler.csZTSharp/ZeroTier/Internal/ZeroTierDataplanePeerDatagramProcessor.csZTSharp/ZeroTier/Internal/ZeroTierDataplanePeerSecurity.csZTSharp/ZeroTier/Internal/ZeroTierDataplaneRootClient.csZTSharp/ZeroTier/Internal/ZeroTierDataplaneRouteRegistry.csZTSharp/ZeroTier/Internal/ZeroTierDataplaneRuntime.csZTSharp/ZeroTier/Internal/ZeroTierDataplaneRuntimeFactory.csZTSharp/ZeroTier/Internal/ZeroTierDataplaneRxLoops.csZTSharp/ZeroTier/Internal/ZeroTierDecryptingPacketReceiver.csZTSharp/ZeroTier/Internal/ZeroTierDirectEndpointManager.csZTSharp/ZeroTier/Internal/ZeroTierDirectEndpointSelection.csZTSharp/ZeroTier/Internal/ZeroTierExternalSurfaceAddressTracker.csZTSharp/ZeroTier/Internal/ZeroTierHelloClient.csZTSharp/ZeroTier/Internal/ZeroTierHelloOkParser.csZTSharp/ZeroTier/Internal/ZeroTierIdentityStore.csZTSharp/ZeroTier/Internal/ZeroTierIpAddressCanonicalization.csZTSharp/ZeroTier/Internal/ZeroTierIpv4Link.csZTSharp/ZeroTier/Internal/ZeroTierIpv4LinkReceiver.csZTSharp/ZeroTier/Internal/ZeroTierIpv4LinkSender.csZTSharp/ZeroTier/Internal/ZeroTierMulticastGatherClient.csZTSharp/ZeroTier/Internal/ZeroTierMulticastLikeClient.csZTSharp/ZeroTier/Internal/ZeroTierNetworkConfigClient.csZTSharp/ZeroTier/Internal/ZeroTierNetworkConfigProtocol.csZTSharp/ZeroTier/Internal/ZeroTierPeerBondPolicyEngine.csZTSharp/ZeroTier/Internal/ZeroTierPeerEchoManager.csZTSharp/ZeroTier/Internal/ZeroTierPeerKeyCache.csZTSharp/ZeroTier/Internal/ZeroTierPeerPathNegotiationManager.csZTSharp/ZeroTier/Internal/ZeroTierPeerPhysicalPathTracker.csZTSharp/ZeroTier/Internal/ZeroTierPeerQosManager.csZTSharp/ZeroTier/Internal/ZeroTierRoutedIpv4Link.csZTSharp/ZeroTier/Internal/ZeroTierRoutedIpv6Link.csZTSharp/ZeroTier/Internal/ZeroTierSelectedPeerPath.csZTSharp/ZeroTier/Internal/ZeroTierSocketBindings.csZTSharp/ZeroTier/Internal/ZeroTierSocketFactory.csZTSharp/ZeroTier/Internal/ZeroTierSocketIdentityMigration.csZTSharp/ZeroTier/Internal/ZeroTierSocketRuntimeBootstrapper.csZTSharp/ZeroTier/Internal/ZeroTierSocketStatePersistence.csZTSharp/ZeroTier/Internal/ZeroTierSocketTcpConnector.csZTSharp/ZeroTier/Internal/ZeroTierTimeouts.csZTSharp/ZeroTier/Internal/ZeroTierWhoisClient.csZTSharp/ZeroTier/Net/Ipv4Codec.csZTSharp/ZeroTier/Net/Ipv6Codec.csZTSharp/ZeroTier/Net/TcpCodec.csZTSharp/ZeroTier/Net/UdpCodec.csZTSharp/ZeroTier/Net/UserSpaceTcpClient.csZTSharp/ZeroTier/Net/UserSpaceTcpReceiveLoop.csZTSharp/ZeroTier/Net/UserSpaceTcpReceiver.csZTSharp/ZeroTier/Net/UserSpaceTcpRemoteSendWindow.csZTSharp/ZeroTier/Net/UserSpaceTcpSender.csZTSharp/ZeroTier/Net/UserSpaceTcpServerConnection.csZTSharp/ZeroTier/Net/UserSpaceTcpServerReceiveLoop.csZTSharp/ZeroTier/Net/ZeroTierFlowId.csZTSharp/ZeroTier/Protocol/ZeroTierPacketCompression.csZTSharp/ZeroTier/Protocol/ZeroTierPacketCrypto.csZTSharp/ZeroTier/Protocol/ZeroTierPacketCryptoAesGmacSiv.csZTSharp/ZeroTier/Protocol/ZeroTierProtocolLimits.csZTSharp/ZeroTier/Protocol/ZeroTierWorldCodec.csZTSharp/ZeroTier/Sockets/ManagedSocket.csZTSharp/ZeroTier/Sockets/ManagedSocketBackend.csZTSharp/ZeroTier/Sockets/ManagedTcpSocketBackend.csZTSharp/ZeroTier/Transport/IZeroTierUdpTransport.csZTSharp/ZeroTier/Transport/ZeroTierUdpDatagram.csZTSharp/ZeroTier/Transport/ZeroTierUdpLocalSocket.csZTSharp/ZeroTier/Transport/ZeroTierUdpMultiTransport.csZTSharp/ZeroTier/Transport/ZeroTierUdpTransport.csZTSharp/ZeroTier/ZeroTierMultipathOptions.csZTSharp/ZeroTier/ZeroTierSocket.csZTSharp/ZeroTier/ZeroTierSocketOptions.csZTSharp/ZeroTier/ZeroTierTcpListener.csZTSharp/ZeroTier/ZeroTierUdpSocket.csdocs/COMPATIBILITY.mddocs/PERSISTENCE.mddocs/Tasks/Fixes2/Plan.mddocs/Tasks/Fixes2/Tracking.mddocs/Tasks/zerotier-multipath-bonding.mddocs/USAGE.mddocs/ZEROTIER_SOCKETS.mddocs/ZEROTIER_STACK.mdsamples/ZTSharp.Cli/CliHelp.cssamples/ZTSharp.Cli/CliParsing.cssamples/ZTSharp.Cli/Commands/CallCommand.cssamples/ZTSharp.Cli/Commands/ExposeCommand.cssamples/ZTSharp.Cli/Commands/JoinCommand.cssamples/ZTSharp.Cli/Commands/ListenCommand.cssamples/ZTSharp.Cli/Commands/UdpListenCommand.cssamples/ZTSharp.Cli/Commands/UdpSendCommand.cs
Comment on lines
+161
to
+163
| await using var _ = await acceptTask.WaitAsync(TimeSpan.FromSeconds(2)); | ||
|
|
||
| var (connectionId, sourcePort) = await synInfoTcs.Task.WaitAsync(TimeSpan.FromSeconds(2)); |
There was a problem hiding this comment.
Keep the accepted server connection alive until after frame delivery.
Line 161 disposes the accepted connection immediately, which can close the session before Lines 168-192 complete and make this test timing-sensitive.
💡 Proposed fix
- await using var _ = await acceptTask.WaitAsync(TimeSpan.FromSeconds(2));
+ await using var acceptedServerClient = await acceptTask.WaitAsync(TimeSpan.FromSeconds(2));
var (connectionId, sourcePort) = await synInfoTcs.Task.WaitAsync(TimeSpan.FromSeconds(2));
@@
var read = await StreamTestHelpers.ReadExactAsync(clientStream, buffer, buffer.Length, cts.Token);
Assert.Equal(buffer.Length, read);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ZTSharp.Tests/ChannelWriterConcurrencyTests.cs` around lines 161 - 163, The
accepted server connection is disposed immediately by "await using var _ = await
acceptTask.WaitAsync(...)" which can close the session before the subsequent
frame delivery (the synInfoTcs.Task.WaitAsync and the logic around lines
168-192) completes; keep the accepted connection alive until after frames are
delivered by removing the immediate await-using disposal: await the acceptTask
to get the connection into a named variable (e.g., acceptedConnection) without
"await using", perform synInfoTcs.Task.WaitAsync(...) and the frame delivery
assertions, then dispose/await dispose the acceptedConnection (or use await
using) only after those checks so the session remains open during the rest of
the test.
Comment on lines
+28
to
+32
| await node1.StartAsync(); | ||
| await node2.StartAsync(); | ||
| await node1.JoinNetworkAsync(networkId); | ||
| await node2.JoinNetworkAsync(networkId); | ||
|
|
There was a problem hiding this comment.
Prime peer mapping first; otherwise spoof checks can be false-positive.
Lines 28-32 and 82-85 only join the network. Without first establishing a legitimate peer endpoint mapping, these tests can pass because unknown sources are dropped before endpoint mismatch validation is reached.
💡 Proposed test-strengthening pattern
@@
await node1.JoinNetworkAsync(networkId);
await node2.JoinNetworkAsync(networkId);
+
+ // Prime peer endpoint mapping using legitimate traffic first.
+ // Then inject spoofed packet from a different socket endpoint.
@@
- using (var spoofUdp = CreateSpoofUdp(node2Endpoint!))
+ using (var spoofUdp = CreateSpoofUdp(node2Endpoint!))
{
await spoofUdp.SendAsync(frameBuffer.AsMemory(0, frameLength), node2Endpoint!);
}Apply the same priming step pattern in both spoofing tests before the forged send.
Also applies to: 82-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ZTSharp.Tests/OsUdpSpoofingTests.cs` around lines 28 - 32, Tests currently
only call StartAsync and JoinNetworkAsync for node1/node2, so endpoint mappings
are never established and spoof checks can false-pass; add a priming step after
JoinNetworkAsync that sends a legitimate packet between the peers (e.g., have
node1 send a normal message to node2 using the existing send method or helper)
to establish the peer endpoint mapping before performing the forged/spoofed
send, and apply this same priming step in both spoofing tests (the blocks around
node1/node2 StartAsync/JoinNetworkAsync and the second occurrence around the
other test) so endpoint validation runs against an existing mapping.
Comment on lines
+41
to
+50
| mgr.RecordOutgoingPacket(peerNodeId, localSocketId: 1, endpoint, packetId: 4); | ||
|
|
||
| now = 1_500; | ||
| Span<byte> payload = stackalloc byte[8 + 2]; | ||
| BinaryPrimitives.WriteUInt64LittleEndian(payload.Slice(0, 8), 4UL); | ||
| BinaryPrimitives.WriteUInt16LittleEndian(payload.Slice(8, 2), 200); | ||
|
|
||
| mgr.HandleInboundMeasurement(peerNodeId, localSocketId: 1, endpoint, payload); | ||
|
|
||
| Assert.False(mgr.TryGetLastLatencyAverageMs(peerNodeId, localSocketId: 1, endpoint, out _)); |
There was a problem hiding this comment.
Untracked test currently uses a tracked packet id.
Line 41 records packet id 4, and Line 45 sends inbound measurement for 4 as well. That does not exercise an untracked path.
💡 Proposed fix
- BinaryPrimitives.WriteUInt64LittleEndian(payload.Slice(0, 8), 4UL);
+ BinaryPrimitives.WriteUInt64LittleEndian(payload.Slice(0, 8), 999UL);📝 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
| mgr.RecordOutgoingPacket(peerNodeId, localSocketId: 1, endpoint, packetId: 4); | |
| now = 1_500; | |
| Span<byte> payload = stackalloc byte[8 + 2]; | |
| BinaryPrimitives.WriteUInt64LittleEndian(payload.Slice(0, 8), 4UL); | |
| BinaryPrimitives.WriteUInt16LittleEndian(payload.Slice(8, 2), 200); | |
| mgr.HandleInboundMeasurement(peerNodeId, localSocketId: 1, endpoint, payload); | |
| Assert.False(mgr.TryGetLastLatencyAverageMs(peerNodeId, localSocketId: 1, endpoint, out _)); | |
| mgr.RecordOutgoingPacket(peerNodeId, localSocketId: 1, endpoint, packetId: 4); | |
| now = 1_500; | |
| Span<byte> payload = stackalloc byte[8 + 2]; | |
| BinaryPrimitives.WriteUInt64LittleEndian(payload.Slice(0, 8), 999UL); | |
| BinaryPrimitives.WriteUInt16LittleEndian(payload.Slice(8, 2), 200); | |
| mgr.HandleInboundMeasurement(peerNodeId, localSocketId: 1, endpoint, payload); | |
| Assert.False(mgr.TryGetLastLatencyAverageMs(peerNodeId, localSocketId: 1, endpoint, out _)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ZTSharp.Tests/ZeroTierPeerQosManagerTests.cs` around lines 41 - 50, The test
is supposed to exercise the untracked path but currently records packet id 4 via
RecordOutgoingPacket and then writes 4 into the inbound payload, so the packet
is tracked; change the packet id mismatch so the inbound measurement uses a
different id than the one recorded. Concretely, modify either the
RecordOutgoingPacket call (e.g., keep it at 4) or the payload write in
HandleInboundMeasurement (BinaryPrimitives.WriteUInt64LittleEndian call) so they
differ (e.g., write 5UL into the payload), then keep the rest of the test and
assert TryGetLastLatencyAverageMs still returns false. Ensure you update the
referenced symbols RecordOutgoingPacket, HandleInboundMeasurement, and the
WriteUInt64LittleEndian payload value accordingly.
Comment on lines
+23
to
+26
| if (IsReparsePoint(_rootPathTrimmed)) | ||
| { | ||
| throw new InvalidOperationException("State root path must not be a symlink/junction/reparse point."); | ||
| } |
There was a problem hiding this comment.
Constructor reparse-point validation misses ancestor components.
Line [23] validates only _rootPathTrimmed. A configured path can still traverse an ancestor symlink/junction (e.g., <base>/<junction>/state) while passing this check if the terminal directory itself is not a reparse point.
🔧 Proposed fix
- if (IsReparsePoint(_rootPathTrimmed))
- {
- throw new InvalidOperationException("State root path must not be a symlink/junction/reparse point.");
- }
+ var cursor = _rootPathTrimmed;
+ while (!string.IsNullOrEmpty(cursor))
+ {
+ if (IsReparsePoint(cursor))
+ {
+ throw new InvalidOperationException("State root path must not traverse a symlink/junction/reparse point.");
+ }
+
+ var parent = Path.GetDirectoryName(cursor);
+ if (string.IsNullOrEmpty(parent) || string.Equals(parent, cursor, _pathComparison))
+ {
+ break;
+ }
+
+ cursor = parent;
+ }📝 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
| if (IsReparsePoint(_rootPathTrimmed)) | |
| { | |
| throw new InvalidOperationException("State root path must not be a symlink/junction/reparse point."); | |
| } | |
| var cursor = _rootPathTrimmed; | |
| while (!string.IsNullOrEmpty(cursor)) | |
| { | |
| if (IsReparsePoint(cursor)) | |
| { | |
| throw new InvalidOperationException("State root path must not traverse a symlink/junction/reparse point."); | |
| } | |
| var parent = Path.GetDirectoryName(cursor); | |
| if (string.IsNullOrEmpty(parent) || string.Equals(parent, cursor, _pathComparison)) | |
| { | |
| break; | |
| } | |
| cursor = parent; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ZTSharp/FileStateStore.cs` around lines 23 - 26, The constructor currently
only checks IsReparsePoint(_rootPathTrimmed) which misses ancestor reparse
points; update the FileStateStore constructor to walk the path hierarchy from
_rootPathTrimmed up to the filesystem root (e.g., using
DirectoryInfo/Directory.GetParent or Path.GetFullPath and iterating parents) and
call IsReparsePoint on each component, throwing the same
InvalidOperationException if any ancestor is a reparse point; keep using the
existing IsReparsePoint helper and _rootPathTrimmed symbol so the change is
limited to adding the ancestor-check loop before proceeding.
Comment on lines
+14
to
+26
| try | ||
| { | ||
| File.SetUnixFileMode(path, UnixFileMode.UserRead | UnixFileMode.UserWrite); | ||
| } | ||
| catch (PlatformNotSupportedException) | ||
| { | ||
| } | ||
| catch (IOException) | ||
| { | ||
| } | ||
| catch (UnauthorizedAccessException) | ||
| { | ||
| } |
There was a problem hiding this comment.
Don’t silently suppress secret-permission hardening failures.
If hardening fails, callers currently get no signal and may continue with weak file permissions. This weakens security guarantees around persisted secrets.
Proposed fix
internal static class SecretFilePermissions
{
- public static void TryHardenSecretFile(string path)
+ public static bool TryHardenSecretFile(string path)
{
ArgumentException.ThrowIfNullOrWhiteSpace(path);
if (OperatingSystem.IsWindows())
{
- return;
+ return true;
}
try
{
File.SetUnixFileMode(path, UnixFileMode.UserRead | UnixFileMode.UserWrite);
+ return true;
}
catch (PlatformNotSupportedException)
{
+ return false;
}
catch (IOException)
{
+ return false;
}
catch (UnauthorizedAccessException)
{
+ return false;
}
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ZTSharp/Internal/SecretFilePermissions.cs` around lines 14 - 26, The current
try/catch around File.SetUnixFileMode in SecretFilePermissions silently swallows
PlatformNotSupportedException/IOException/UnauthorizedAccessException so callers
never know permission hardening failed; update the block that calls
File.SetUnixFileMode(path, UnixFileMode.UserRead | UnixFileMode.UserWrite) to
surface failures to callers by removing the empty catches for IOException and
UnauthorizedAccessException (or rethrow/wrap them into a clear exception like
InvalidOperationException or IOException with the original exception as
InnerException and a message like "failed to set secret file permissions for
{path}"), and keep only an explicit handling decision for
PlatformNotSupportedException if you intentionally want to ignore platforms
without Unix file modes — otherwise propagate that too; ensure the change is
made in the SecretFilePermissions code path that invokes File.SetUnixFileMode so
callers can react to permission-hardening failures.
Comment on lines
+116
to
121
| if (_finSeq is not null) | ||
| { | ||
| throw new IOException("Local has closed the connection."); | ||
| } | ||
|
|
||
| await _sendLock.WaitAsync(cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
Move the FIN guard inside _sendLock in WriteAsync.
Line 116 checks _finSeq before lock acquisition at Line 121. A concurrent SendFinWithRetriesAsync can set _finSeq while WriteAsync is waiting, which allows post-FIN data sends.
🔧 Proposed fix
- if (_finSeq is not null)
- {
- throw new IOException("Local has closed the connection.");
- }
-
await _sendLock.WaitAsync(cancellationToken).ConfigureAwait(false);
try
{
+ if (_finSeq is not null)
+ {
+ throw new IOException("Local has closed the connection.");
+ }
+
var remaining = buffer;
while (!remaining.IsEmpty)
{📝 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
| if (_finSeq is not null) | |
| { | |
| throw new IOException("Local has closed the connection."); | |
| } | |
| await _sendLock.WaitAsync(cancellationToken).ConfigureAwait(false); | |
| await _sendLock.WaitAsync(cancellationToken).ConfigureAwait(false); | |
| try | |
| { | |
| if (_finSeq is not null) | |
| { | |
| throw new IOException("Local has closed the connection."); | |
| } | |
| var remaining = buffer; | |
| while (!remaining.IsEmpty) | |
| { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ZTSharp/ZeroTier/Net/UserSpaceTcpSender.cs` around lines 116 - 121, In
WriteAsync, move the `_finSeq` guard so it is checked after acquiring
`_sendLock` (i.e., inside the critical section protected by `_sendLock`) to
prevent a race where `SendFinWithRetriesAsync` sets `_finSeq` while WriteAsync
is waiting; specifically, in the WriteAsync method (class UserSpaceTcpSender)
acquire `_sendLock` first via `await _sendLock.WaitAsync(...)` and then check
`_finSeq` and throw the IOException if non-null, ensuring all send-path checks
and state mutations are done under the same lock.
Comment on lines
+191
to
212
| await InitLock.WaitAsync(token).ConfigureAwait(false); | ||
| try | ||
| { | ||
| ObjectDisposedException.ThrowIf(Disposed, this); | ||
| _stream = stream; | ||
| _remoteEndPoint = remoteIp; | ||
| _localEndPoint = localEndpoint; | ||
| } | ||
| catch | ||
| { | ||
| await stream.DisposeAsync().ConfigureAwait(false); | ||
| throw; | ||
| } | ||
| finally | ||
| { | ||
| InitLock.Release(); | ||
| } | ||
| } | ||
| catch (OperationCanceledException) when (ShutdownToken.IsCancellationRequested) | ||
| { | ||
| throw new ObjectDisposedException(GetType().FullName); | ||
| } |
There was a problem hiding this comment.
Potential resource leak if cancellation occurs between connect and state assignment.
If cancellation occurs at InitLock.WaitAsync(token) (line 191) after the stream has been obtained (lines 187-189), the stream won't be disposed because the catch block (lines 199-203) only catches exceptions thrown within the inner try block (lines 193-197).
The catch (OperationCanceledException) at line 209 doesn't dispose the stream before throwing ObjectDisposedException.
🛡️ Suggested fix: dispose stream on outer cancellation
catch (OperationCanceledException) when (ShutdownToken.IsCancellationRequested)
{
+ if (stream is not null)
+ {
+ await stream.DisposeAsync().ConfigureAwait(false);
+ }
throw new ObjectDisposedException(GetType().FullName);
}Note: This requires hoisting the stream variable declaration outside the inner try block scope, or restructuring the flow to ensure cleanup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ZTSharp/ZeroTier/Sockets/ManagedTcpSocketBackend.cs` around lines 191 - 212,
The code can leak the obtained stream if OperationCanceledException occurs
between acquiring the stream and assigning
_stream/_remoteEndPoint/_localEndPoint; hoist the local stream variable
declaration (the one assigned from the connect call) outside the inner try so
it’s visible to the outer catch, and in the outer catch (the
OperationCanceledException when ShutdownToken.IsCancellationRequested) ensure
you DisposeAsync/Dispose the hoisted stream before rethrowing the
ObjectDisposedException; keep using InitLock.WaitAsync(token), the inner
try/catch for assignment, and preserve InitLock.Release in finally while adding
the cleanup of stream in the outer cancellation handler to guarantee no resource
leak.
Comment on lines
+76
to
+82
| if (_joinTask is { IsCompleted: true } && !_joined) | ||
| { | ||
| _joinTask = null; | ||
| } | ||
|
|
||
| _joinTask ??= JoinCoreAsync(_shutdown.Token); | ||
| } |
There was a problem hiding this comment.
Capture _joinTask / _runtimeTask into locals before unlocking.
Both methods await mutable fields after releasing locks. Concurrent resets can produce NullReferenceException or await a different task than the one selected in the critical section.
🧵 Proposed race fix
- try
+ Task joinTask;
+ try
{
ThrowIfDisposed();
if (_joined)
{
return;
@@
if (_joinTask is { IsCompleted: true } && !_joined)
{
_joinTask = null;
}
_joinTask ??= JoinCoreAsync(_shutdown.Token);
+ joinTask = _joinTask;
}
@@
- await _joinTask.WaitAsync(token).ConfigureAwait(false);
+ await joinTask.WaitAsync(token).ConfigureAwait(false);- try
+ Task<ZeroTierDataplaneRuntime> runtimeTask;
+ try
{
ThrowIfDisposed();
if (_runtime is not null)
{
return _runtime;
@@
if (_runtimeTask is { IsCompleted: true })
{
_runtimeTask = null;
}
_runtimeTask ??= CreateRuntimeAsync(inlineCom, _shutdown.Token);
+ runtimeTask = _runtimeTask;
}
@@
- return await _runtimeTask.WaitAsync(token).ConfigureAwait(false);
+ return await runtimeTask.WaitAsync(token).ConfigureAwait(false);Also applies to: 88-91, 374-380, 386-389
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ZTSharp/ZeroTier/ZeroTierSocket.cs` around lines 76 - 82, Concurrent resets
of mutable fields cause races because the code awaits fields like _joinTask and
_runtimeTask after releasing locks; capture the task into a local variable
inside the lock and await that local outside the lock instead. Specifically, in
the code paths that assign/nullable-reset _joinTask and _runtimeTask (and where
you call JoinCoreAsync(_shutdown.Token) or the runtime core starter), do: inside
the lock determine/assign the field and immediately copy it to a local (e.g.,
var joinTask = _joinTask; var runtimeTask = _runtimeTask), release the lock,
then await the local; apply the same pattern to the other occurrences noted (the
blocks around _joinTask/_joined and the runtime task blocks) so you never await
a mutable field after the lock is released.
Comment on lines
+398
to
+413
| var (created, helloOk, rootKey) = await ZeroTierSocketRuntimeBootstrapper | ||
| .CreateAsync( | ||
| multipath: _options.Multipath, | ||
| localIdentity: _identity, | ||
| planet: _planet, | ||
| networkId: _options.NetworkId, | ||
| managedIps: ManagedIps, | ||
| inlineCom: inlineCom, | ||
| cachedRoot: _upstreamRoot, | ||
| cachedRootKey: _upstreamRootKey, | ||
| cancellationToken) | ||
| .ConfigureAwait(false); | ||
|
|
||
| cancellationToken.ThrowIfCancellationRequested(); | ||
| ThrowIfDisposed(); | ||
|
|
There was a problem hiding this comment.
Dispose created runtime on cancellation/disposal races.
If bootstrap succeeds but ThrowIfDisposed/cancellation triggers before ownership is committed to _runtime, the newly created runtime is leaked.
♻️ Proposed ownership-safe disposal pattern
- var (created, helloOk, rootKey) = await ZeroTierSocketRuntimeBootstrapper
- .CreateAsync(
+ ZeroTierDataplaneRuntime? created = null;
+ try
+ {
+ var bootstrap = await ZeroTierSocketRuntimeBootstrapper
+ .CreateAsync(
multipath: _options.Multipath,
localIdentity: _identity,
planet: _planet,
networkId: _options.NetworkId,
managedIps: ManagedIps,
inlineCom: inlineCom,
cachedRoot: _upstreamRoot,
cachedRootKey: _upstreamRootKey,
cancellationToken)
- .ConfigureAwait(false);
+ .ConfigureAwait(false);
+
+ created = bootstrap.Runtime;
+ var helloOk = bootstrap.HelloOk;
+ var rootKey = bootstrap.RootKey;
- cancellationToken.ThrowIfCancellationRequested();
- ThrowIfDisposed();
+ cancellationToken.ThrowIfCancellationRequested();
+ ThrowIfDisposed();
- ZeroTierDataplaneRuntime? toDispose = null;
- ZeroTierDataplaneRuntime runtime;
- await _runtimeLock.WaitAsync(cancellationToken).ConfigureAwait(false);
- try
- {
- ThrowIfDisposed();
- if (_runtime is not null)
- {
- toDispose = created;
- runtime = _runtime;
- }
- else
- {
- _upstreamRoot ??= helloOk;
- _upstreamRootKey ??= rootKey;
- _runtime = created;
- runtime = created;
- }
- }
- finally
- {
- _runtimeLock.Release();
- }
+ ZeroTierDataplaneRuntime? toDispose = null;
+ ZeroTierDataplaneRuntime runtime;
+ await _runtimeLock.WaitAsync(cancellationToken).ConfigureAwait(false);
+ try
+ {
+ ThrowIfDisposed();
+ if (_runtime is not null)
+ {
+ toDispose = created;
+ runtime = _runtime;
+ }
+ else
+ {
+ _upstreamRoot ??= helloOk;
+ _upstreamRootKey ??= rootKey;
+ _runtime = created;
+ runtime = created;
+ created = null; // ownership transferred
+ }
+ }
+ finally
+ {
+ _runtimeLock.Release();
+ }
- if (toDispose is not null)
- {
- await toDispose.DisposeAsync().ConfigureAwait(false);
- }
+ if (toDispose is not null)
+ {
+ await toDispose.DisposeAsync().ConfigureAwait(false);
+ }
- return runtime;
+ return runtime;
+ }
+ finally
+ {
+ if (created is not null)
+ {
+ await created.DisposeAsync().ConfigureAwait(false);
+ }
+ }Also applies to: 414-444
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ZTSharp/ZeroTier/ZeroTierSocket.cs` around lines 398 - 413, CreateAsync
returns a created runtime that can be leaked if cancellation or ThrowIfDisposed
races before assigning to the field; after awaiting
ZeroTierSocketRuntimeBootstrapper.CreateAsync (the tuple assigned to local
"created, helloOk, rootKey"), immediately check
cancellationToken.IsCancellationRequested and this.ThrowIfDisposed(), and if
either is true dispose/cleanup the "created" runtime (call its Dispose/Shutdown
method) before throwing/returning; otherwise atomically assign "created" to the
instance field "_runtime" to transfer ownership. Apply the same pattern for the
second creation site (lines 414-444): capture the created runtime in a local,
guard with cancellation/ThrowIfDisposed, dispose on race, and only then set
_runtime.
Comment on lines
+183
to
+186
| if (Interlocked.Exchange(ref _disposeState, 1) != 0) | ||
| { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.cs" | head -20Repository: JKamsker/ZTSharp
Length of output: 888
🏁 Script executed:
fd 'ZeroTierUdpSocket.cs|ZeroTierDataplaneRuntime.cs' -t fRepository: JKamsker/ZTSharp
Length of output: 151
🏁 Script executed:
rg -n 'ZeroTierUdpSocket' --type=cs -lRepository: JKamsker/ZTSharp
Length of output: 379
🏁 Script executed:
fd -t f -e cs | xargs rg -l 'UnregisterUdpPort' 2>/dev/null | head -5Repository: JKamsker/ZTSharp
Length of output: 211
🏁 Script executed:
wc -l ZTSharp/ZeroTier/ZeroTierUdpSocket.csRepository: JKamsker/ZTSharp
Length of output: 101
🏁 Script executed:
sed -n '170,220p' ZTSharp/ZeroTier/ZeroTierUdpSocket.csRepository: JKamsker/ZTSharp
Length of output: 1425
🏁 Script executed:
rg -n 'UnregisterUdpPort' ZTSharp/ZeroTier/Internal/ZeroTierDataplaneRuntime.cs -A 10 -B 2Repository: JKamsker/ZTSharp
Length of output: 881
🏁 Script executed:
rg -n '_disposeState|_disposeLock' ZTSharp/ZeroTier/ZeroTierUdpSocket.cs | head -20Repository: JKamsker/ZTSharp
Length of output: 364
🏁 Script executed:
rg -n 'class ZeroTierDataplaneRouteRegistry' ZTSharp/ZeroTier/Internal/ZeroTierDataplaneRouteRegistry.cs -A 50 | head -60Repository: JKamsker/ZTSharp
Length of output: 2422
🏁 Script executed:
rg -n 'UnregisterUdpPort' ZTSharp/ZeroTier/Internal/ZeroTierDataplaneRouteRegistry.cs -B 2 -A 10Repository: JKamsker/ZTSharp
Length of output: 489
🏁 Script executed:
cat ZTSharp.Tests/ZeroTierUdpSocketDisposeTests.csRepository: JKamsker/ZTSharp
Length of output: 1613
Ensure _disposeLock is disposed even if cleanup throws.
The current code skips line 205 (_disposeLock.Dispose()) if an exception occurs during cleanup (lines 193–201). Because the finally block only releases the semaphore but does not dispose it, the SemaphoreSlim is leaked. Additionally, _disposeState remains stuck at 1, preventing retry if cleanup fails.
While UnregisterUdpPort and TryComplete() are unlikely to throw in normal operation, defensive structure is needed to guarantee cleanup. Wrap the lock acquisition and cleanup in an outer try/finally to ensure disposal:
Proposed fix
if (Interlocked.Exchange(ref _disposeState, 1) != 0)
{
return;
}
-await _disposeLock.WaitAsync().ConfigureAwait(false);
-try
-{
- if (_disposed)
- {
- return;
- }
-
- _disposed = true;
- _runtime.UnregisterUdpPort(_localAddress.AddressFamily, _localPort);
- _incoming.Writer.TryComplete();
-}
-finally
-{
- _disposeLock.Release();
-}
-
-_disposeLock.Dispose();
+try
+{
+ await _disposeLock.WaitAsync().ConfigureAwait(false);
+ try
+ {
+ if (_disposed)
+ {
+ return;
+ }
+
+ _disposed = true;
+ _runtime.UnregisterUdpPort(_localAddress.AddressFamily, _localPort);
+ _incoming.Writer.TryComplete();
+ }
+ finally
+ {
+ _disposeLock.Release();
+ }
+}
+catch
+{
+ Interlocked.Exchange(ref _disposeState, 0);
+ throw;
+}
+finally
+{
+ _disposeLock.Dispose();
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ZTSharp/ZeroTier/ZeroTierUdpSocket.cs` around lines 183 - 186, The disposal
path currently sets _disposeState via Interlocked.Exchange and acquires
_disposeLock but if cleanup (calls like UnregisterUdpPort or TryComplete) throws
the code returns before calling _disposeLock.Dispose(), leaking the
SemaphoreSlim and leaving _disposeState stuck at 1; modify
ZeroTierUdpSocket.Dispose so that after Interlocked.Exchange(ref _disposeState,
1) you wrap the lock acquisition and cleanup in an outer try/finally: in the try
do the existing await _disposeLock.WaitAsync() and perform UnregisterUdpPort and
TryComplete, and in the outer finally always call _disposeLock.Release() (if
held) and _disposeLock.Dispose() to guarantee disposal; additionally, if cleanup
fails, reset _disposeState back to 0 inside the catch so future dispose attempts
can retry (or rethrow after resetting) — reference the fields/methods
_disposeState, _disposeLock, UnregisterUdpPort, TryComplete, and ensure
SemaphoreSlim is disposed no matter what.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests