[PR #2930] [MERGED] fix: stack overflow while resolving site routes #3018

New issue

Closed

opened 2026-10-14 21:06:55 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-14 21:06:55 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/amnezia-vpn/amnezia-client/pull/2930
Author: @romanio427
Created: 8/3/2026
Status: ✅ Merged
Merged: 8/5/2026
Merged by: @vkamn

Base: dev ← Head: fix/site-routes-dns-stack-overflow

📝 Commits (1)

f6d5e26 fix: avoid reentrant DNS flush while resolving site routes

📊 Changes

1 file changed (+28 additions, -7 deletions)

View changed files

📝 client/vpnConnection.cpp (+28 -7)

📄 Description

Summary

keep multi-IP site resolution from #2811 (addVpnSite(..., QStringList))
install each resolved site route as soon as its lookup completes
track outstanding hostname lookups and flush DNS once when they all finish
flush DNS asynchronously through QRemoteObjectPendingCallWatcher (no nested event loop)
capture gw / ip by value in the async routeAddList lambda (avoids dangling refs from the previous [&gw, &ip] capture)

Problem

VpnConnection::addSitesRoutes() synchronously called QRemoteObjectPendingCall::waitForFinished() from every asynchronous hostname lookup callback.

waitForFinished() processes a nested Qt event loop. With a large site split-tunneling list, another queued QHostInfo callback can run before the previous callback returns. The cycle repeats until AmneziaVPN.exe terminates with 0xc00000fd (stack overflow).

A Windows crash dump showed this repeated stack cycle approximately 66 times:

addSitesRoutes callback -> flushDns -> waitForFinished -> QEventLoop::exec -> next QHostInfo callback

The old condition also logged failure on successful completion because it used:

reply.waitForFinished() || !reply.returnValue()

The asynchronous watcher preserves error reporting without starting nested event loops.

What changed in this update

Rebased onto current dev after #2811 (fix: enhance split tunneling IP handling) landed in the same function and produced a merge conflict in client/vpnConnection.cpp.

Conflict resolution keeps both sides:

From #2811 (`dev`)	From this PR
Resolve all IPv4 addresses per site	Do not call `waitForFinished()` in the `QHostInfo` callback
`addVpnSite(mode, site, newIps)` with `QStringList`	Count remaining lookups; flush DNS once at the end
Split-tunneling debug log	Async flush via `QRemoteObjectPendingCallWatcher`
Flush only when new IPs were installed	Same policy via shared `needFlush` flag

Reproduction and dump evidence

Observed environment:

Windows 11, build 26100.8655
AmneziaVPN 5.0.0.5
Xray connection using tun2socks
Kill switch and site split tunneling enabled
A large site list producing more than 9,000 route entries during the incident

Reproduction:

Enable site split tunneling with the large site list.
Connect the Xray configuration.
While the hostnames are being re-resolved, AmneziaVPN.exe exits with 0xc00000fd.

The dump inspection showed this logical frame cycle repeated approximately 66 times before the stack overflow:

VpnConnection::addSitesRoutes QHostInfo callback
  -> IpcInterfaceReplica::flushDns
  -> QRemoteObjectPendingCall::waitForFinished
  -> nested QEventLoop::exec
  -> next queued QHostInfo callback
  -> VpnConnection::addSitesRoutes QHostInfo callback

The original dump is no longer available for attachment, so the cycle above is a transcription of the inspected call pattern rather than a raw debugger export. The patch removes the nested synchronous wait from this callback path.

Verification notes

Merge conflicts with dev are resolved; PR is mergeable against current dev.
Change is scoped to desktop (#ifdef AMNEZIA_DESKTOP); the new include is under the same guard.
Static review: flushDns() is SLOT(bool flushDns()), so QRemoteObjectPendingReply<bool> + QRemoteObjectPendingCallWatcher match the existing Qt Remote Objects IPC stack.
No local packaged Windows build was produced for this PR. Runtime confirmation still needs: large site list + split tunneling + connect/re-resolve on Windows (the original crash scenario).

Linked issue

Fixes #2938

Scope

This PR only fixes the client crash during site-route resolution. Restoring or cleaning up an orphaned desktop VPN session after an unexpected GUI exit is a separate lifecycle concern.

Related to #2748 — the same Windows stack-overflow exception (0xc00000fd in Qt6Core.dll) after connecting, followed by the GUI exiting and a network interface remaining active with non-functional DNS. This PR addresses the identified stack-overflow path only; it does not claim to fix the separate AmneziaDNS/reinstallation behavior reported there.
Related to #1258 — tun2socks and its tunnel survive abnormal application or system termination, leaving no internet and preventing a new connection. This is the same orphaned-session aftermath, not the crash root cause fixed by this PR.
Related to #2152 — after restart, the desktop GUI reports Disconnected while the VPN is still active. This matches the client/service state mismatch observed after our crash.
Follow-up: #2931 contains the current Windows reproduction and source-level analysis of the orphaned-session lifecycle gap. It is intentionally separate because this PR only fixes the triggering stack overflow.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/amnezia-vpn/amnezia-client/pull/2930 **Author:** [@romanio427](https://github.com/romanio427) **Created:** 8/3/2026 **Status:** ✅ Merged **Merged:** 8/5/2026 **Merged by:** [@vkamn](https://github.com/vkamn) **Base:** `dev` ← **Head:** `fix/site-routes-dns-stack-overflow` --- ### 📝 Commits (1) - [`f6d5e26`](https://github.com/amnezia-vpn/amnezia-client/commit/f6d5e2661244abc23bab4f69415bf2543da08702) fix: avoid reentrant DNS flush while resolving site routes ### 📊 Changes **1 file changed** (+28 additions, -7 deletions) <details> <summary>View changed files</summary> 📝 `client/vpnConnection.cpp` (+28 -7) </details> ### 📄 Description ## Summary - keep multi-IP site resolution from #2811 (`addVpnSite(..., QStringList)`) - install each resolved site route as soon as its lookup completes - track outstanding hostname lookups and flush DNS **once** when they all finish - flush DNS asynchronously through `QRemoteObjectPendingCallWatcher` (no nested event loop) - capture `gw` / `ip` by value in the async `routeAddList` lambda (avoids dangling refs from the previous `[&gw, &ip]` capture) ## Problem `VpnConnection::addSitesRoutes()` synchronously called `QRemoteObjectPendingCall::waitForFinished()` from every asynchronous hostname lookup callback. `waitForFinished()` processes a nested Qt event loop. With a large site split-tunneling list, another queued `QHostInfo` callback can run before the previous callback returns. The cycle repeats until `AmneziaVPN.exe` terminates with `0xc00000fd` (stack overflow). A Windows crash dump showed this repeated stack cycle approximately 66 times: `addSitesRoutes callback -> flushDns -> waitForFinished -> QEventLoop::exec -> next QHostInfo callback` The old condition also logged failure on successful completion because it used: `reply.waitForFinished() || !reply.returnValue()` The asynchronous watcher preserves error reporting without starting nested event loops. ## What changed in this update Rebased onto current `dev` after #2811 (`fix: enhance split tunneling IP handling`) landed in the same function and produced a merge conflict in `client/vpnConnection.cpp`. Conflict resolution keeps both sides: | From #2811 (`dev`) | From this PR | | --- | --- | | Resolve **all** IPv4 addresses per site | Do **not** call `waitForFinished()` in the `QHostInfo` callback | | `addVpnSite(mode, site, newIps)` with `QStringList` | Count remaining lookups; flush DNS **once** at the end | | Split-tunneling debug log | Async flush via `QRemoteObjectPendingCallWatcher` | | Flush only when new IPs were installed | Same policy via shared `needFlush` flag | ## Reproduction and dump evidence Observed environment: - Windows 11, build 26100.8655 - AmneziaVPN 5.0.0.5 - Xray connection using `tun2socks` - Kill switch and site split tunneling enabled - A large site list producing more than 9,000 route entries during the incident Reproduction: 1. Enable site split tunneling with the large site list. 2. Connect the Xray configuration. 3. While the hostnames are being re-resolved, `AmneziaVPN.exe` exits with `0xc00000fd`. The dump inspection showed this logical frame cycle repeated approximately 66 times before the stack overflow: ```text VpnConnection::addSitesRoutes QHostInfo callback -> IpcInterfaceReplica::flushDns -> QRemoteObjectPendingCall::waitForFinished -> nested QEventLoop::exec -> next queued QHostInfo callback -> VpnConnection::addSitesRoutes QHostInfo callback ``` The original dump is no longer available for attachment, so the cycle above is a transcription of the inspected call pattern rather than a raw debugger export. The patch removes the nested synchronous wait from this callback path. ## Verification notes - Merge conflicts with `dev` are resolved; PR is mergeable against current `dev`. - Change is scoped to desktop (`#ifdef AMNEZIA_DESKTOP`); the new include is under the same guard. - Static review: `flushDns()` is `SLOT(bool flushDns())`, so `QRemoteObjectPendingReply<bool>` + `QRemoteObjectPendingCallWatcher` match the existing Qt Remote Objects IPC stack. - No local packaged Windows build was produced for this PR. Runtime confirmation still needs: large site list + split tunneling + connect/re-resolve on Windows (the original crash scenario). ## Linked issue Fixes #2938 ## Scope This PR only fixes the client crash during site-route resolution. Restoring or cleaning up an orphaned desktop VPN session after an unexpected GUI exit is a separate lifecycle concern. ## Related issues - Related to #2748 — the same Windows stack-overflow exception (`0xc00000fd` in `Qt6Core.dll`) after connecting, followed by the GUI exiting and a network interface remaining active with non-functional DNS. This PR addresses the identified stack-overflow path only; it does not claim to fix the separate AmneziaDNS/reinstallation behavior reported there. - Related to #1258 — `tun2socks` and its tunnel survive abnormal application or system termination, leaving no internet and preventing a new connection. This is the same orphaned-session aftermath, not the crash root cause fixed by this PR. - Related to #2152 — after restart, the desktop GUI reports Disconnected while the VPN is still active. This matches the client/service state mismatch observed after our crash. - Follow-up: #2931 contains the current Windows reproduction and source-level analysis of the orphaned-session lifecycle gap. It is intentionally separate because this PR only fixes the triggering stack overflow. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-14 21:06:55 +00:00