Skip to content

WireGuard: Improve config error handling; Prevent panic in case of errors during server initialization #4566

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

IlyaGulya
Copy link
Contributor

This pull request:

  1. Properly wraps config building errors with context-rich error messages
  2. Adds validation for empty WireGuard keys with clear error messages
  3. Implements proper resource cleanup in WireGuard TUN device with a sync.Once to prevent double-close issues leading to panic
  4. Adds test case to verify WireGuard server initialization handles errors gracefully without panicking

Overall, this leads to much better user experience in case of misconfiguration

@Fangliding
Copy link
Member

1 应该使用项目内部的errors包
2 旧的代码在什么使用情况下会导致崩溃?

@IlyaGulya
Copy link
Contributor Author

  1. Ok, I will fix
  2. For example, here I forgot to nest the protocol-specific settings in settings field, this will lead to secretKey being empty and this will lead to panic.
{
  "inbounds": [
    {
      "listen": "127.0.0.1",
      "port": "50120",
      "tag": "wg",
      "protocol": "wireguard",
      "secretKey": "SOMEKEY",
      "peers": [
        {
          "publicKey": "SOMEKEY"
        }
      ]
    }
  ]
}```

@Fangliding
Copy link
Member

这在infra/conf部分处理就行了 我的意思是后面tun.go的部分

@IlyaGulya IlyaGulya force-pushed the bugfix/panic-because-of-double-channel-close-during-wireguard-initialization branch from 282836a to 34555e3 Compare March 28, 2025 19:05
@IlyaGulya
Copy link
Contributor Author

Fixed to use internal errors package.
Regarding the tun.go fix. My example was only about the wrong secretKey, but I suppose that any IPC error during the initialization would lead to the panic.
I suppose there's a better way to handle it, like fixing the double close, but this was just the simpliest fix I've come up with

- Replace generic errors with context-rich messages using fmt.Errorf and %w
- Add validation for empty WireGuard keys with clear error messages
- Implement proper resource cleanup in WireGuard TUN with sync.Once
- Add test to verify WireGuard server handles initialization errors without panic
@IlyaGulya IlyaGulya force-pushed the bugfix/panic-because-of-double-channel-close-during-wireguard-initialization branch from 34555e3 to 0ffd7e1 Compare March 29, 2025 07:00
@RPRX RPRX requested a review from Fangliding March 29, 2025 15:29
@RPRX
Copy link
Member

RPRX commented Mar 30, 2025

看了下代码,就是加了详细的配置错误说明,Close() 内加了个 sync.Once 类似 quic-go/quic-go#4798 ,加了个 test,可以合并

WireGuard 这部分代码不是很重要,属于是谁愿意改谁改,改出 bug 的话再修,反正不是很重要

@RPRX RPRX changed the title Improve error handling and prevent WireGuard panic in case of errors during server initialization WireGuard: Improve config error handling; Prevent panic in case of errors during server initialization Mar 30, 2025
@RPRX RPRX merged commit 543de6a into XTLS:main Mar 30, 2025
35 checks passed
@RPRX
Copy link
Member

RPRX commented Mar 30, 2025

我都改了标题 GitHub 还背刺?写 release notes 时还得手动加 first contribution,烦死了我草

RPRX pushed a commit that referenced this pull request Mar 30, 2025
@IlyaGulya IlyaGulya deleted the bugfix/panic-because-of-double-channel-close-during-wireguard-initialization branch March 31, 2025 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants