fix: UDP port allocation race causing link self-loop (one-way links)

PortManager.get_free_udp_port had an unguarded find-then-add sequence. A link allocates both ends concurrently (asyncio.gather in UDPLink._prepare -> two POST /ports/udp) and FastAPI runs the sync route handler in a threadpool, so both threads could probe the same 'free' port before either recorded it — handing lport == rport to both ends. uBridge sets SO_REUSEADDR on UDP NIO sockets, so the double bind succeeds silently and the kernel delivers everything to the last-bound socket: one node starves, the other echoes to itself. Make every TCP/UDP allocate/reserve/release path atomic with an RLock, and rebuild _link_data in UDPLink._prepare so reset() commits the fresh port pair instead of re-sending the stale, already-released one. Regression tests: threaded barrier allocation never returns duplicates (red on the old code, UDP and TCP); reset() leaves exactly one mirrored NIO pair per side with lport != rport (red on the old code).
2026-08-27 12:30:13 +03:00 · 2026-08-14 21:31:58 +08:00 · 2026-08-14 21:31:58 +08:00 · 350f2b24e7
commit 350f2b24e7
parent 1cddeb8c3f
7 changed files with 285 additions and 89 deletions
--- a/docs/README.md
+++ b/docs/README.md
@ -109,7 +109,7 @@ Cisco XRd as a GNS3 Docker router: vendor path + shm/device injection (`GNS3_SHM
 ## Known Issues (`bugs/`)

 - [Telnet Server Connection Race Condition](bugs/telnet-server-connection-race-condition.md) — `getpeername()` error when client disconnects during connection setup (High severity, Open)
- [Docker Link UDP Self-Loop](bugs/link-udp-self-loop.md) — intermittent one-way link (one end's NIO points at itself); delete/recreate the link as workaround (Medium severity, Open)
+- [Docker Link UDP Self-Loop](bugs/link-udp-self-loop.md) — intermittent one-way link (both ends handed the same UDP port by an allocation race); **fixed** (Medium severity)

 ---

--- a/docs/bugs/link-udp-self-loop.md
+++ b/docs/bugs/link-udp-self-loop.md
@ -5,25 +5,24 @@ See LICENSE file for licensing information.

 > This documentation is organized by AI with reference to actual code. AI can make mistakes — please verify against the source code when in doubt.

-
 # Docker Link UDP Self-Loop Bug (One-Way Link)

 ## Bug Report

 **Date**: 2026-08-14
 **Severity**: Medium (one-way connectivity, CPU burn from packet duplication; intermittent)
-**Status**: Open — root cause not yet pinpointed; workaround reliable
-**Component**: Link wiring — `gns3server/controller/udp_link.py` (`_prepare` /
-`pop_preallocated_udp_port` in `gns3server/controller/project.py`) interacting with
-node/uBridge restarts
+**Status**: **Fixed** — root cause found and unit-tested (same day)
+**Component**: UDP port allocation — `gns3server/compute/port_manager.py`
+(`get_free_udp_port` find-then-add race); secondary: `gns3server/controller/udp_link.py`
+(`_prepare` accumulated stale `_link_data` on reset)

 ## Symptoms

-Two Docker nodes (observed with Cisco XRd; likely node-type agnostic) linked on the
+Two Docker nodes (observed with Cisco XRd; node-type agnostic) linked on the
 same compute cannot ping each other. Packet capture on the link shows only **one**
 side sending ARP. The other side's traffic never appears on the link at all.

-## Evidence (from a live occurrence)
+## Evidence (from the live occurrence)

 Captured simultaneously on both nodes' uBridge `bridge1` (see diagnostics below):

@ -34,23 +33,44 @@ Captured simultaneously on both nodes' uBridge `bridge1` (see diagnostics below)
 | B's `ethN` counters | RX ≈ TX ≈ 5000+ — B receives its own transmissions back |
 | A's `ethN` counters | TX > 0, RX = 0 — never receives anything |

-Conclusion: **B's `nio_udp` rport pointed at B's own lport** — a UDP self-loop. B's
-uBridge had restarted ~77 s after A's (node stop/start during the session),
-i.e. the link was re-wired at least once.
+## Root Cause (confirmed)

-Deleting and re-creating the link fixed it immediately (fresh port allocation).
+**`PortManager.get_free_udp_port` had an unguarded find-then-add sequence.**
+A link allocates the UDP port for **both ends concurrently**
+(`asyncio.gather` in `UDPLink._prepare` → two `POST /ports/udp`). The route
+handler is a sync `def`, so FastAPI executes the two requests **in parallel
+threads**. Both threads ran `find_unused_port` (socket-probing, GIL-releasing)
+before either reached `_used_udp_ports.add(port)` — the set add is idempotent,
+so no error was raised and **both ends were handed the same port number**:
+`lport == rport` on both NIOs, a literal self-loop.

-## Root Cause Analysis (narrowed, not final)
+Why it was *silent* and *asymmetric*:

- `UDPLink._prepare()` builds mirrored NIO data correctly
-  (`node1: lport=P1/rport=P2`, `node2: lport=P2/rport=P1`) — the logic itself is sound.
- Suspects for the corrupted runtime state:
-  1. `Project.pop_preallocated_udp_port()` — the batch project-open preallocation
-     (link-create performance work) racing with link re-creation;
-  2. link re-creation racing a node/uBridge restart (commit NIOs to a uBridge that is
-     being torn down/rebuilt), leaving a stale/self-pointing NIO on one side.
- A deterministic reproduction is still needed: create two Docker nodes + link,
-  restart one node, then verify the UDP wiring (see diagnostics).
+- uBridge sets `SO_REUSEADDR` on UDP NIO sockets (`ubridge/src/nio_udp.c`), so
+  the second bind of the same port **succeeds** instead of failing with
+  `EADDRINUSE` — link creation returned success.
+- With two sockets bound to the same port, the kernel delivers to one of them
+  (last bound wins). The node that started later — in the live case B,
+  restarted ~77 s after A — received **everything**: A's packets *and* its own
+  transmissions echoed back. The 77 s restart did not cause the corruption; it
+  only decided which end starves.
+- The same-compute condition is part of the trigger: both allocations hit the
+  same `PortManager` instance (a cross-compute link races two processes and
+  cannot self-collide).
+
+A second, smaller defect was found while auditing: `UDPLink._prepare()`
+**appended** to `self._link_data` but the committed NIOs are always taken from
+indices 0/1 — after `reset()` (delete + create on the same object) the stale,
+already-released port pair was re-committed and the freshly allocated ports
+were leaked.
+
+## Fix
+
+| Change | Where |
+|---|---|
+| `threading.RLock` making find-then-add (and reserve/release) atomic for TCP and UDP | `gns3server/compute/port_manager.py` |
+| `_prepare()` rebuilds `_link_data` from scratch instead of appending | `gns3server/controller/udp_link.py` |
+| Regression tests: threaded allocation never returns duplicates (red on the old code); `reset()` commits the fresh mirrored pair with `lport != rport` | `tests/compute/test_port_manager.py`, `tests/controller/test_udp_link.py` |

 ## Diagnostics (uBridge console is the fast path)

@ -63,8 +83,10 @@ Deleting and re-creating the link fixed it immediately (fresh port allocation).
 2. **Container counters** — `docker exec <cid> ip -s link show ethN`:
   TX>0/RX=0 → peer never returns; RX≈TX huge with µs-scale duplicates → self-loop.
 3. **UDP sockets** — `ss -uln` (no `-p`; uBridge runs setuid-root so process names
-   are hidden, ports are visible): a two-node link owns exactly two "orphan" UDP ports.
-4. **Workaround** — delete and re-create the link (or stop/start both nodes).
+   are hidden, ports are visible): a two-node link owns exactly two "orphan" UDP
+   ports. **One port instead of two = this bug.**
+4. **Recovery** — delete and re-create the link (or stop/start both nodes);
+   with the fix, the corruption no longer occurs in the first place.

 Note: a Docker node's in-container `ethN` is a **TAP device** whose file descriptor
 lives inside uBridge (the interface is created host-side, then moved into the
@ -74,4 +96,6 @@ waste time hunting for one in the host namespace.
 ## Related

 - `docs/features/vendor-nos-xrd.md` — troubleshooting table entry pointing here.
- Link-create batch optimization (`controller/udp_link.py`, project-open bulk path).
+- Link-create batch optimization (`controller/udp_link.py`, project-open bulk path)
+  was exonerated: the pool path allocates sequentially in one handler and cannot
+  self-collide.
--- a/docs/features/vendor-nos-xrd.md
+++ b/docs/features/vendor-nos-xrd.md
@ -158,7 +158,7 @@ sequenceDiagram
 | `Invalid interface entries ... XR_MGMT_INTERFACES` | use `xr_name=Mg0/RP0/CPU0/0` |
 | Host audio muted / USB reconnects when the node starts | set `GNS3_MASK_UDEV=1` |
 | Config lost across stop/start | `extra_volumes` must be `/xr-storage-shadow` |
-| Two nodes can't ping, only one side ARPs | GNS3 link wiring bug (UDP self-loop on one end), not XRd — delete and re-create the link; see [link-udp-self-loop](../bugs/link-udp-self-loop.md) |
+| Two nodes can't ping, only one side ARPs | GNS3 link wiring bug (UDP self-loop on one end), not XRd — fixed (port-allocation race); on older builds delete and re-create the link; see [link-udp-self-loop](../bugs/link-udp-self-loop.md) |
 | Compute log: busybox coredump storm | fixed by the container-chown change; verify gns3-server is current |

 ## Notes
@ -191,5 +191,6 @@ sequenceDiagram

 | Version | Date | Changes |
 |---------|------|---------|
+| 1.2 | 2026-08-14 | Link UDP self-loop root-caused to a port-allocation race and fixed — see [link-udp-self-loop](../bugs/link-udp-self-loop.md) (now marked Fixed). |
 | 1.1 | 2026-08-14 | Datapath validated end-to-end (XRd brings its own interfaces up; ARP/ICMP bidirectional). Add troubleshooting entry for the one-way-link symptom (GNS3 link UDP self-loop bug, see bugs/link-udp-self-loop.md). |
 | 1.0 | 2026-08-14 | Initial documentation of the XRd control-plane adaptation: vendor path requirement, shm/devices/extra_configs/udev-mask mechanisms, host-disturbance root causes, appliance recipe. |
--- a/gns3server/compute/port_manager.py
+++ b/gns3server/compute/port_manager.py
@ -16,6 +16,7 @@

 import socket
 import ipaddress
+import threading
 from fastapi import HTTPException, status
 from gns3server.config import Config

@ -105,6 +106,13 @@ class PortManager:
        self._udp_host = "0.0.0.0"
        self._used_tcp_ports = set()
        self._used_udp_ports = set()
+        # Guards the find-then-add port allocation against concurrent threads:
+        # FastAPI runs sync route handlers (e.g. POST /ports/udp) in a thread
+        # pool, and a link allocates both of its ends concurrently — without
+        # the lock both threads can probe the same "free" port and hand the
+        # same number to both ends of a link (lport == rport self-loop).
+        # RLock because reserve_*_port falls back to get_free_*_port.
+        self._lock = threading.RLock()

        console_start_port_range = Config.instance().settings.Server.console_start_port_range
        console_end_port_range = Config.instance().settings.Server.console_end_port_range
@ -275,16 +283,17 @@ class PortManager:
            port_range_start = self._console_port_range[0]
            port_range_end = self._console_port_range[1]

-        port = self.find_unused_port(
-            port_range_start,
-            port_range_end,
-            host=self._console_host,
-            socket_type="TCP",
-            ignore_ports=self._used_tcp_ports,
-        )
+        with self._lock:
+            port = self.find_unused_port(
+                port_range_start,
+                port_range_end,
+                host=self._console_host,
+                socket_type="TCP",
+                ignore_ports=self._used_tcp_ports,
+            )

-        self._used_tcp_ports.add(port)
-        project.record_tcp_port(port)
+            self._used_tcp_ports.add(port)
+            project.record_tcp_port(port)
        log.debug(f"TCP port {port} has been allocated")
        return port

@ -305,32 +314,33 @@ class PortManager:
            port_range_start = self._console_port_range[0]
            port_range_end = self._console_port_range[1]

-        if port in self._used_tcp_ports:
-            old_port = port
-            port = self.get_free_tcp_port(project, port_range_start=port_range_start, port_range_end=port_range_end)
-            msg = f"TCP port {old_port} already in use on host {self._console_host}. Port has been replaced by {port}"
-            log.debug(msg)
-            return port
-        if port < port_range_start or port > port_range_end:
-            old_port = port
-            port = self.get_free_tcp_port(project, port_range_start=port_range_start, port_range_end=port_range_end)
-            msg = (
-                f"TCP port {old_port} is outside the range {port_range_start}-{port_range_end} on host "
-                f"{self._console_host}. Port has been replaced by {port}"
-            )
-            log.debug(msg)
-            return port
-        try:
-            PortManager._check_port(self._console_host, port, "TCP")
-        except OSError:
-            old_port = port
-            port = self.get_free_tcp_port(project, port_range_start=port_range_start, port_range_end=port_range_end)
-            msg = f"TCP port {old_port} already in use on host {self._console_host}. Port has been replaced by {port}"
-            log.debug(msg)
-            return port
+        with self._lock:
+            if port in self._used_tcp_ports:
+                old_port = port
+                port = self.get_free_tcp_port(project, port_range_start=port_range_start, port_range_end=port_range_end)
+                msg = f"TCP port {old_port} already in use on host {self._console_host}. Port has been replaced by {port}"
+                log.debug(msg)
+                return port
+            if port < port_range_start or port > port_range_end:
+                old_port = port
+                port = self.get_free_tcp_port(project, port_range_start=port_range_start, port_range_end=port_range_end)
+                msg = (
+                    f"TCP port {old_port} is outside the range {port_range_start}-{port_range_end} on host "
+                    f"{self._console_host}. Port has been replaced by {port}"
+                )
+                log.debug(msg)
+                return port
+            try:
+                PortManager._check_port(self._console_host, port, "TCP")
+            except OSError:
+                old_port = port
+                port = self.get_free_tcp_port(project, port_range_start=port_range_start, port_range_end=port_range_end)
+                msg = f"TCP port {old_port} already in use on host {self._console_host}. Port has been replaced by {port}"
+                log.debug(msg)
+                return port

-        self._used_tcp_ports.add(port)
-        project.record_tcp_port(port)
+            self._used_tcp_ports.add(port)
+            project.record_tcp_port(port)
        log.debug(f"TCP port {port} has been reserved")
        return port

@ -342,10 +352,11 @@ class PortManager:
        :param project: Project instance
        """

-        if port in self._used_tcp_ports:
-            self._used_tcp_ports.remove(port)
-            project.remove_tcp_port(port)
-            log.debug(f"TCP port {port} has been released")
+        with self._lock:
+            if port in self._used_tcp_ports:
+                self._used_tcp_ports.remove(port)
+                project.remove_tcp_port(port)
+                log.debug(f"TCP port {port} has been released")

    def get_free_udp_port(self, project):
        """
@ -353,16 +364,17 @@ class PortManager:

        :param project: Project instance
        """
-        port = self.find_unused_port(
-            self._udp_port_range[0],
-            self._udp_port_range[1],
-            host=self._udp_host,
-            socket_type="UDP",
-            ignore_ports=self._used_udp_ports,
-        )
+        with self._lock:
+            port = self.find_unused_port(
+                self._udp_port_range[0],
+                self._udp_port_range[1],
+                host=self._udp_host,
+                socket_type="UDP",
+                ignore_ports=self._used_udp_ports,
+            )

-        self._used_udp_ports.add(port)
-        project.record_udp_port(port)
+            self._used_udp_ports.add(port)
+            project.record_udp_port(port)
        log.debug(f"UDP port {port} has been allocated")
        return port

@ -374,18 +386,20 @@ class PortManager:
        :param project: Project instance
        """

-        if port in self._used_udp_ports:
-            raise HTTPException(
-                status_code=status.HTTP_409_CONFLICT,
-                detail=f"UDP port {port} already in use on host {self._console_host}",
-            )
-        if port < self._udp_port_range[0] or port > self._udp_port_range[1]:
-            raise HTTPException(
-                status_code=status.HTTP_409_CONFLICT,
-                detail=f"UDP port {port} is outside the range " f"{self._udp_port_range[0]}-{self._udp_port_range[1]}",
-            )
-        self._used_udp_ports.add(port)
-        project.record_udp_port(port)
+        with self._lock:
+            if port in self._used_udp_ports:
+                raise HTTPException(
+                    status_code=status.HTTP_409_CONFLICT,
+                    detail=f"UDP port {port} already in use on host {self._console_host}",
+                )
+            if port < self._udp_port_range[0] or port > self._udp_port_range[1]:
+                raise HTTPException(
+                    status_code=status.HTTP_409_CONFLICT,
+                    detail=f"UDP port {port} is outside the range "
+                    f"{self._udp_port_range[0]}-{self._udp_port_range[1]}",
+                )
+            self._used_udp_ports.add(port)
+            project.record_udp_port(port)
        log.debug(f"UDP port {port} has been reserved")

    def release_udp_port(self, port, project):
@ -396,7 +410,8 @@ class PortManager:
        :param project: Project instance
        """

-        if port in self._used_udp_ports:
-            self._used_udp_ports.remove(port)
-            project.remove_udp_port(port)
-            log.debug(f"UDP port {port} has been released")
+        with self._lock:
+            if port in self._used_udp_ports:
+                self._used_udp_ports.remove(port)
+                project.remove_udp_port(port)
+                log.debug(f"UDP port {port} has been released")
--- a/gns3server/controller/udp_link.py
+++ b/gns3server/controller/udp_link.py
@ -98,6 +98,13 @@ class UDPLink(Link):
            tuples, ready to be POSTed to each node's compute.
        """

+        # Start from a clean slate: reset() re-creates the link on the same
+        # object (delete() + create()), and _commit_nios()/update() always
+        # address indices 0/1 — appending onto the previous run's entries
+        # would re-commit the stale (already released) port pair and orphan
+        # the freshly allocated ports.
+        self._link_data = []
+
        node1 = self._nodes[0]["node"]
        adapter_number1 = self._nodes[0]["adapter_number"]
        port_number1 = self._nodes[0]["port_number"]
--- a/tests/compute/test_port_manager.py
+++ b/tests/compute/test_port_manager.py
@ -16,6 +16,7 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.

 import pytest
+import threading
 import uuid

 from fastapi import HTTPException
@ -116,6 +117,82 @@ def test_release_udp_port():
    pm.reserve_udp_port(20000, project)


+def test_concurrent_udp_port_allocation_no_duplicates():
+    """
+    Regression test for the link UDP self-loop bug (docs/bugs/link-udp-self-loop.md):
+    both ends of a link are allocated concurrently on the controller
+    (asyncio.gather -> two POST /ports/udp), and FastAPI runs the sync route
+    handler in a threadpool. The find-then-add allocation must be atomic,
+    otherwise both threads can probe and return the same "free" port —
+    handing lport == rport to both ends, which makes every packet loop back
+    to its sender (one-way link).
+    """
+
+    pm = PortManager()
+    pm.udp_port_range = (50000, 50100)
+    project = Project(project_id=str(uuid.uuid4()))
+
+    workers = 8
+    rounds = 10
+    barrier = threading.Barrier(workers)
+    results = []
+    results_lock = threading.Lock()
+
+    def worker():
+        allocated = []
+        for _ in range(rounds):
+            # start each round together to maximize the collision window
+            barrier.wait()
+            allocated.append(pm.get_free_udp_port(project))
+        with results_lock:
+            results.extend(allocated)
+
+    threads = [threading.Thread(target=worker) for _ in range(workers)]
+    for thread in threads:
+        thread.start()
+    for thread in threads:
+        thread.join()
+
+    assert len(results) == workers * rounds
+    assert len(set(results)) == len(results), "the same UDP port was handed to two callers"
+    assert pm.udp_ports == set(results)
+
+
+def test_concurrent_tcp_port_allocation_no_duplicates():
+    """
+    Same race class as the UDP self-loop bug, on the console/TCP side:
+    concurrent get_free_tcp_port calls must never return the same port.
+    """
+
+    pm = PortManager()
+    pm.console_port_range = (51000, 51100)
+    project = Project(project_id=str(uuid.uuid4()))
+
+    workers = 8
+    rounds = 10
+    barrier = threading.Barrier(workers)
+    results = []
+    results_lock = threading.Lock()
+
+    def worker():
+        allocated = []
+        for _ in range(rounds):
+            barrier.wait()
+            allocated.append(pm.get_free_tcp_port(project))
+        with results_lock:
+            results.extend(allocated)
+
+    threads = [threading.Thread(target=worker) for _ in range(workers)]
+    for thread in threads:
+        thread.start()
+    for thread in threads:
+        thread.join()
+
+    assert len(results) == workers * rounds
+    assert len(set(results)) == len(results), "the same TCP port was handed to two callers"
+    assert pm.tcp_ports == set(results)
+
+
 def test_find_unused_port():

    p = PortManager().find_unused_port(1000, 10000)
--- a/tests/controller/test_udp_link.py
+++ b/tests/controller/test_udp_link.py
@ -187,6 +187,78 @@ async def test_delete(project):
    compute2.delete.assert_any_call("/projects/{}/vpcs/nodes/{}/adapters/3/ports/1/nio".format(project.id, node2.id), timeout=120)


+@pytest.mark.asyncio
+async def test_reset(project):
+    """
+    reset() re-creates the link on the same object: the fresh port pair must
+    replace the stale one instead of accumulating (the committed NIOs always
+    come from indices 0/1) — see docs/bugs/link-udp-self-loop.md.
+    """
+
+    compute1 = MagicMock()
+    compute2 = MagicMock()
+
+    node1 = Node(project, compute1, "node1", node_type="vpcs")
+    node1._ports = [EthernetPort("E0", 0, 0, 4)]
+    node2 = Node(project, compute2, "node2", node_type="vpcs")
+    node2._ports = [EthernetPort("E0", 0, 3, 1)]
+
+    async def subnet_callback(compute2):
+        """
+        Fake subnet callback
+        """
+        return ("192.168.1.1", "192.168.1.2")
+
+    compute1.get_ip_on_same_subnet.side_effect = subnet_callback
+
+    # per-compute port sequences: first create -> 1024/2048, reset -> 4096/8192
+    node1_ports = iter([1024, 4096])
+    node2_ports = iter([2048, 8192])
+
+    async def compute1_callback(path, data={}, **kwargs):
+        if "/ports/udp" in path:
+            response = MagicMock()
+            response.json = {"udp_port": next(node1_ports)}
+            return response
+
+    async def compute2_callback(path, data={}, **kwargs):
+        if "/ports/udp" in path:
+            response = MagicMock()
+            response.json = {"udp_port": next(node2_ports)}
+            return response
+
+    compute1.post.side_effect = compute1_callback
+    compute1.host = "example.com"
+    compute2.post.side_effect = compute2_callback
+    compute2.host = "example.org"
+
+    link = UDPLink(project)
+    await link.add_node(node1, 0, 4)
+    await link.add_node(node2, 3, 1)
+
+    await link.reset()
+
+    # exactly one (fresh) NIO spec per side — no stale entries left behind
+    assert len(link.debug_link_data) == 2
+    assert link.debug_link_data[0]["lport"] == 4096
+    assert link.debug_link_data[0]["rport"] == 8192
+    assert link.debug_link_data[1]["lport"] == 8192
+    assert link.debug_link_data[1]["rport"] == 4096
+    # the self-loop invariant: an end's lport must never equal its rport
+    assert link.debug_link_data[0]["lport"] != link.debug_link_data[0]["rport"]
+    assert link.debug_link_data[1]["lport"] != link.debug_link_data[1]["rport"]
+    # the committed NIO carries the fresh pair, not the released one
+    compute1.post.assert_any_call("/projects/{}/vpcs/nodes/{}/adapters/0/ports/4/nio".format(project.id, node1.id), data={
+        "lport": 4096,
+        "rhost": "192.168.1.2",
+        "rport": 8192,
+        "type": "nio_udp",
+        "filters": {},
+        "markers": {},
+        "suspend": False,
+    }, timeout=120)
+
+
@pytest.mark.asyncio
 async def test_choose_capture_side(project):
    """