Fixes from the Qodo review of the whole codebase (#34); v1.12

.github/workflows/build.yml

@@ -93,6 +93,8 @@ jobs:
           key: vtun-${{ hashFiles('server/build-vtund.sh') }}
 
       - name: Build and run the unit tests under qemu
+        env:
+          GITHUB_TOKEN: ${{ github.token }}   # toolchain digest lookup (ci/build-musl.sh)
         run: QEMU=1 ci/build-musl.sh ${{ matrix.target }} dist
 
       - name: Upload binaries

CHANGELOG.md

@@ -4,6 +4,27 @@ The version lives in `include/vtesse/version.h`. Binaries are published only as
 [`latest`](https://github.com/compot-dev/vtesse/releases/tag/latest) release, rebuilt
 from every push to `main`; this file is the change history.
 
+## 1.12 — 2026-10-15
+
+Fixes from the first Qodo review of the whole codebase (#34, PR #35):
+
+- lwIP backend: a DHCP renewal replaces the classless routes (or removes them) even
+  though the address stays the same; before, only the first lease set them.
+- The keepalive timeout is refreshed only by complete frames: a peer that trickles
+  bytes of a frame it never finishes no longer keeps a dead session alive.
+- `vt_create()` rejects negative timer settings; the reconnect backoff cannot overflow.
+- SOCKS5: an unfinished handshake is dropped after the connect timeout (15 s), so idle
+  clients cannot hold every connection slot.
+- `vtesse-cli`: `--seconds` and the `--fetch` port are checked (`--seconds 1m` used to
+  mean "no limit"); the `stop` hook runs only after earlier hooks finished, so a DHCP
+  client started by a late `up` hook is not left running.
+- `vtesse-udhcpc.script` installs on-link classless routes (gateway 0.0.0.0) without `via`.
+- `S99vtesse` stop/status check the process (`-x`), so a stale PID file cannot make
+  them signal or report another process.
+- CI: unit-test failures under QEMU fail the build (`| tail -1` hid them); toolchain
+  archives are checked against SHA256 before their compiler runs; the bench self-test
+  checks that ECHO_REQ really is the first frame.
+
 ## 1.11 — 2026-10-15
 
 - The Android APK of every new version is posted to the OpenIPC Telegram dev channel

bench/selftest.py

@@ -91,6 +91,24 @@ def wait_flag(s, want, seconds):
     return False
 
 
+def first_frame_flag(s, seconds):
+    """Return the flag of the first frame (0 for a data frame), or None on timeout/EOF."""
+    s.settimeout(seconds)
+    buf = b""
+    try:
+        while len(buf) < 2:
+            c = s.recv(2 - len(buf))
+            if not c:
+                return None
+            buf += c
+    except socket.timeout:
+        return None
+    hdr = struct.unpack(">H", buf)[0]
+    if hdr & 0xF000 == ECHO_REQ:
+        s.sendall(struct.pack(">H", ECHO_REP))
+    return hdr & 0xF000
+
+
 def dhcp_offer(s, mac, seconds):
     """Send DHCPDISCOVER, return (seconds until OFFER, raw frame) or (None, None)."""
     t0 = time.time()
@@ -129,7 +147,7 @@ def main():
     check("greeting is exactly 50 bytes, NUL padded",
           len(g[0]) == 50 and g[0].startswith(b"VTUN server ver ") and g[0].rstrip(b"\0").endswith(b"\n"))
     check("main session flags <TeK>", reply == "OK FLAGS: <TeK>\n", repr(reply))
-    check("ECHO_REQ is the first frame after OK FLAGS", wait_flag(s, ECHO_REQ, 2))
+    check("ECHO_REQ is the first frame after OK FLAGS", first_frame_flag(s, 2) == ECHO_REQ)
     time.sleep(0.5)  # the server brings the tap up asynchronously
     dt, fr = dhcp_offer(s, bytes.fromhex(S["main"][0]), 10)
     check("DHCP OFFER through the tunnel", fr is not None, "after %.1fs" % dt if dt else "")

ci/Dockerfile.cross

@@ -1,6 +1,6 @@
 # Cross build environment for ci/build-musl.sh (Bootlin musl toolchains + qemu-user).
 FROM debian:bookworm-slim
 RUN apt-get update \
- && apt-get install -y --no-install-recommends make curl ca-certificates xz-utils file qemu-user \
+ && apt-get install -y --no-install-recommends make curl ca-certificates xz-utils file qemu-user jq \
  && rm -rf /var/lib/apt/lists/*
 WORKDIR /src

ci/build-musl.sh

@@ -20,6 +20,10 @@ set -eu
 TARGET=$1
 OUT=${2:-dist}
 BOOTLIN_VERSION=${BOOTLIN_VERSION:-stable-2026.08-1}
+# The archives are checked before their compiler is run: SHA256 pinned below for the
+# default Bootlin and OpenWrt versions (TOOLCHAIN_SHA256 overrides it, required with
+# another version); the OpenIPC toolchain is a rolling release asset, checked
+# against the digest GitHub reports for it.
 CACHE=${TOOLCHAIN_CACHE:-$HOME/.cache/vtesse-toolchains}
 ROOT=$(cd "$(dirname "$0")/.." && pwd)
 
@@ -29,23 +33,32 @@ OPENWRT_VERSION=${OPENWRT_VERSION:-23.05.5}
 OPENWRT=https://downloads.openwrt.org/releases/$OPENWRT_VERSION/targets
 SUBDIR=     # toolchain directory inside the archive, if not its top level
 case "$TARGET" in
-    arm32)   URL=$OPENIPC/toolchain.hisilicon-hi3516cv100.tgz; DIRNAME=arm-openipc-linux-musleabi_sdk-buildroot; QEMU_BIN=qemu-arm ;;
+    arm32)   URL=$OPENIPC/toolchain.hisilicon-hi3516cv100.tgz; DIRNAME=arm-openipc-linux-musleabi_sdk-buildroot; QEMU_BIN=qemu-arm
+             SHA=github ;;
     mipsel|mips)
         # OpenWrt toolchains: musl, soft-float (24kc), as on the routers themselves
         if [ "$TARGET" = mipsel ]; then OW=ramips/mt7621; OWA=mipsel_24kc; QEMU_BIN=qemu-mipsel
-        else OW=ath79/generic; OWA=mips_24kc; QEMU_BIN=qemu-mips; fi
+            SHA=bb08478a46869009a2a8b85f88e266cc6b6d3ccefc34de61907ead2a986e08c9
+        else OW=ath79/generic; OWA=mips_24kc; QEMU_BIN=qemu-mips
+            SHA=6cf197b2705307057548e65158396ca4d26b343f557bf81c97fd5643978cc991; fi
+        [ "$OPENWRT_VERSION" = 23.05.5 ] || SHA=
         DIRNAME=openwrt-toolchain-$OPENWRT_VERSION-$(echo $OW | tr / -)_gcc-12.3.0_musl.Linux-x86_64
         URL=$OPENWRT/$OW/$DIRNAME.tar.xz
         SUBDIR=/toolchain-${OWA}_gcc-12.3.0_musl ;;
     *)
         case "$TARGET" in
-            x86_64) TC=x86-64;       QEMU_BIN=qemu-x86_64 ;;
-            armv5)  TC=armv5-eabi;   QEMU_BIN=qemu-arm ;;
-            armv7)  TC=armv7-eabihf; QEMU_BIN=qemu-arm ;;
-            arm64)  TC=aarch64;      QEMU_BIN=qemu-aarch64 ;;
+            x86_64) TC=x86-64;       QEMU_BIN=qemu-x86_64
+                    SHA=78d3a4683d6ac47b5ee73bd5bce210b55eb93dff1b137c61298af97eb0d2b5a6 ;;
+            armv5)  TC=armv5-eabi;   QEMU_BIN=qemu-arm
+                    SHA=a7db571eaa0f1ba2eea9a3932e8ad64bfaf33bfb7e7b96dd09702fe3ecedcf1a ;;
+            armv7)  TC=armv7-eabihf; QEMU_BIN=qemu-arm
+                    SHA=9147bafae4aa272321a3c6440d04d83b7e23411b2d344f875541d84c4444ba9b ;;
+            arm64)  TC=aarch64;      QEMU_BIN=qemu-aarch64
+                    SHA=b388c480a48e8e9f9b99e3d14e69219c4d61e5a2424a82faecb88a015b781a60 ;;
             *) echo "unknown target: $TARGET" >&2; exit 2 ;;
         esac
         DIRNAME=$TC--musl--$BOOTLIN_VERSION
+        [ "$BOOTLIN_VERSION" = stable-2026.08-1 ] || SHA=
         URL=$BOOTLIN/$TC/tarballs/$DIRNAME.tar.xz ;;
 esac
 
@@ -56,6 +69,20 @@ if [ ! -d "$DIR/bin" ]; then
     # download first (with retries), then extract: a truncated stream piped
     # into tar looks like a corrupt archive
     curl -fsSL --retry 3 -o "$CACHE/toolchain.tar" "$URL"
+    SHA=${TOOLCHAIN_SHA256:-$SHA}
+    if [ "$SHA" = github ]; then
+        # authenticated when a token is available: anonymous calls share a 60/hour limit per IP
+        command -v jq >/dev/null ||
+            { echo "jq is needed to verify $URL (or set TOOLCHAIN_SHA256)" >&2; rm -f "$CACHE/toolchain.tar"; exit 1; }
+        TOKEN=${GITHUB_TOKEN:-${GH_TOKEN:-}}
+        REL=$(curl -fsSL --retry 3 ${TOKEN:+-H "Authorization: Bearer $TOKEN"} \
+              "https://api.github.com/repos/OpenIPC/firmware/releases/tags/toolchain") ||
+            { echo "GitHub API request for the toolchain digest failed (rate limit?)" >&2; rm -f "$CACHE/toolchain.tar"; exit 1; }
+        SHA=$(printf '%s' "$REL" | jq -r --arg n "${URL##*/}" '.assets[] | select(.name == $n) | .digest // "" | sub("^sha256:"; "")')
+    fi
+    [ -n "$SHA" ] || { echo "no SHA256 for $URL: set TOOLCHAIN_SHA256" >&2; rm -f "$CACHE/toolchain.tar"; exit 1; }
+    echo "$SHA  $CACHE/toolchain.tar" | sha256sum -c - >/dev/null ||
+        { echo "SHA256 mismatch for $URL" >&2; rm -f "$CACHE/toolchain.tar"; exit 1; }
     tar -xf "$CACHE/toolchain.tar" -C "$CACHE"
     rm -f "$CACHE/toolchain.tar"
     [ -x "$DIR/relocate-sdk.sh" ] && "$DIR/relocate-sdk.sh" >/dev/null 2>&1 || true
@@ -94,7 +121,9 @@ fi
 if [ "${QEMU:-0}" = 1 ]; then
     for t in test_proto test_route test_client; do
         echo "== $QEMU_BIN $t"
-        "$QEMU_BIN" "build-$TARGET-full/$t" | tail -1
+        # no pipefail in sh: keep the test's own exit status, not that of tail
+        out=$("$QEMU_BIN" "build-$TARGET-full/$t" 2>&1) || { echo "$out"; echo "$t failed under $QEMU_BIN" >&2; exit 1; }
+        echo "$out" | tail -1
     done
     "$QEMU_BIN" "$OUT/vtesse-$TARGET" --version
     "$QEMU_BIN" "$OUT/vtesse-tap-$TARGET" --version

cli/vtesse_cli.c

@@ -14,6 +14,7 @@
  */
 #include <errno.h>
 #include <signal.h>
+#include <time.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -315,7 +316,11 @@ static int parse_url(const char *url)
     fetch.port = 80;
     colon = strchr(host, ':');
     if (colon) {
-        fetch.port = (uint16_t)atoi(colon + 1);
+        char *end;
+        long port = strtol(colon + 1, &end, 10);
+        if (end == colon + 1 || *end || port < 1 || port > 65535)
+            return -1;
+        fetch.port = (uint16_t)port;
         host[colon - host] = '\0';
     }
     snprintf(fetch.path, sizeof(fetch.path), "%s", slash ? slash : "/");
@@ -515,16 +520,78 @@ static void reap_children(void)
         ;
 }
 
+/* Hooks still running, by PID: as PID 1 (in a container) we also inherit the DHCP
+ * client the hook daemonized, so waiting for "any child" would never return. */
+static pid_t hook_pids[16];
+
+/* 1: the hook has exited (or was already reaped), 0: still running. EINTR is
+ * retried, so an interrupted check never counts as an exit. */
+static int hook_done(pid_t pid)
+{
+    pid_t r;
+
+    while ((r = waitpid(pid, NULL, WNOHANG)) < 0 && errno == EINTR)
+        ;
+    return r != 0;      /* pid, or -1 with ECHILD */
+}
+
+/* Waits for the hooks started so far: "stop" must not overtake an earlier "up" that
+ * has yet to start the DHCP client (it would be left running after we exit). A hook
+ * still running after HOOK_WAIT_MS is terminated, so shutdown cannot hang on it. */
+#define HOOK_WAIT_MS 5000
+
+static void wait_hooks(void)
+{
+    uint64_t deadline = nc_now_ms() + HOOK_WAIT_MS;
+    size_t i;
+
+    for (i = 0; i < sizeof(hook_pids) / sizeof(hook_pids[0]); i++) {
+        while (hook_pids[i] > 0) {
+            if (hook_done(hook_pids[i])) {
+                hook_pids[i] = 0;               /* finished, or already reaped */
+            } else if (nc_now_ms() >= deadline) {
+                uint64_t grace = nc_now_ms() + 1000;
+                printf("[%8.3f] hook: pid %d still running, terminated\n", elapsed(t_start),
+                       (int)hook_pids[i]);
+                /* the whole group: also a udhcpc still in the hook's foreground */
+                kill(-hook_pids[i], SIGTERM);
+                while (!hook_done(hook_pids[i]) && nc_now_ms() < grace) {
+                    struct timespec ts = { 0, 20 * 1000000L };
+                    nanosleep(&ts, NULL);
+                }
+                kill(-hook_pids[i], SIGKILL);
+                while (waitpid(hook_pids[i], NULL, 0) < 0 && errno == EINTR)
+                    ;
+                hook_pids[i] = 0;
+            } else {
+                struct timespec ts = { 0, 20 * 1000000L };
+                nanosleep(&ts, NULL);
+            }
+        }
+    }
+}
+
 /* Runs "$hook <event>" asynchronously with VTESSE_* in the environment. */
 static void run_hook(const char *event)
 {
     pid_t pid;
+    size_t slot;
 
     reap_children();
     if (!hook)
         return;
+    /* a free slot first: every hook we start must be one shutdown can wait for */
+    for (slot = 0; slot < sizeof(hook_pids) / sizeof(hook_pids[0]); slot++)
+        if (hook_pids[slot] <= 0 || hook_done(hook_pids[slot]))
+            break;
+    if (slot == sizeof(hook_pids) / sizeof(hook_pids[0])) {
+        printf("[%8.3f] hook: too many hooks still running, \"%s\" not started\n",
+               elapsed(t_start), event);
+        return;
+    }
     pid = fork();
     if (pid == 0) {
+        setpgid(0, 0);      /* own process group, so a stuck hook can be ended as a whole */
         setenv("VTESSE_EVENT", event, 1);
         setenv("VTESSE_IFNAME", nc_tap_ifname(tap), 1);
         setenv("VTESSE_LINK", nc_tap_link(tap) == TUNNEL_LINK_L3 ? "tun" : "ether", 1);
@@ -534,6 +601,10 @@ static void run_hook(const char *event)
     }
     if (pid < 0)
         printf("[%8.3f] hook: fork failed: %s\n", elapsed(t_start), strerror(errno));
+    else {
+        setpgid(pid, pid);  /* also here: no race with a kill() before the child ran */
+        hook_pids[slot] = pid;
+    }
 }
 
 static void on_tap_event(void *user, int ev, int tev, int err)
@@ -598,8 +669,16 @@ int main(int argc, char **argv)
     for (i = 1; i < argc; i++) {
         if (!strcmp(argv[i], "--config") && i + 1 < argc)
             config = argv[++i];
-        else if (!strcmp(argv[i], "--seconds") && i + 1 < argc)
-            seconds = atof(argv[++i]);
+        else if (!strcmp(argv[i], "--seconds") && i + 1 < argc) {
+            const char *arg = argv[++i];
+            char *end;
+            seconds = strtod(arg, &end);
+            /* "1m" or "-5" must not mean "forever"; inf or 1e30 would overflow the deadline */
+            if (end == arg || *end || !(seconds > 0) || !(seconds <= 1e9)) {
+                fprintf(stderr, "--seconds: expected a positive number of seconds up to 1e9, got '%s'\n", arg);
+                return 2;
+            }
+        }
         else if (!strcmp(argv[i], "-v"))
             verbose = 1;
         else if (!strcmp(argv[i], "--id-from") && i + 1 < argc)
@@ -889,9 +968,10 @@ int main(int argc, char **argv)
                nc_tap_ifname(tap), ts.frames_in, ts.frames_out, ts.dropped);
         if (nc_tap_link(tap) == TUNNEL_LINK_NONE && vt_is_up(vt))
             rc = 1;     /* interface creation failed */
+        wait_hooks();
         run_hook("stop");
+        wait_hooks();
         nc_tap_destroy(tap);
-        reap_children();
     }
 #endif
     t.ops->close(vt);

include/vtesse/version.h

@@ -3,6 +3,6 @@
 
 /* MAJOR.MM. Shown by --version and in the title of the "latest" release (CI);
  * record each bump in CHANGELOG.md. */
-#define VTESSE_VERSION "1.11"
+#define VTESSE_VERSION "1.12"
 
 #endif

netcore/proxy.c

@@ -549,6 +549,8 @@ static void accept_on(nc_proxy_t *px, int which)
         if (which == 0) {
             c->socks = 1;
             c->state = C_GREETING;
+            /* the handshake gets the connect timeout too: idle clients must not hold slots */
+            c->deadline = nc_now_ms() + (uint64_t)px->cfg.connect_timeout_ms;
         } else {
             const nc_forward_t *f = &px->cfg.forwards[which - 1];
             c->dst = f->ip;
@@ -639,7 +641,7 @@ static int px_next_timeout(void *ctx)
 
     for (i = 0; i < px->nconns; i++) {
         const conn_t *c = px->conns[i];
-        if (c->state == C_CONNECTING && c->deadline && (!next || c->deadline < next))
+        if (c->state <= C_CONNECTING && !c->dead && c->deadline && (!next || c->deadline < next))
             next = c->deadline;
     }
     if (!next)
@@ -659,6 +661,10 @@ static void px_timer(void *ctx)
             struct tcp_pcb *pcb = c->pcb;
             c->deadline = 0;
             tcp_abort(pcb);         /* calls on_err(ERR_ABRT) -> "connect timeout" */
+        } else if (c->state < C_CONNECTING && !c->dead && c->deadline && now >= c->deadline) {
+            logf_(px, "socks: handshake timeout");
+            c->deadline = 0;
+            c->dead = 1;
         }
     }
     service(px);