Skip to content
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

Add local response to ICMP PING requests (IPv4 & IPv6) #5

Open
wants to merge 1 commit into
base: shadowsocks-android
Choose a base branch
from

Conversation

RPRX
Copy link

@RPRX RPRX commented Apr 25, 2021

2dust/v2rayNG#1027

高效实现,十分感谢 @yuhan6665 协助测试

@RPRX
Copy link
Author

RPRX commented Apr 25, 2021

一些 APP 会使用 ping 来检测连通性,而 socks 不支持代理 icmp 流量,tun2socks 此前的行为则是不处理、不响应

@RPRX
Copy link
Author

RPRX commented Apr 25, 2021

这个 PR 非常高效地实现了对 ping 请求的本地响应,支持 IPv4 和 IPv6,使这些 APP 可以正常工作

@Mygod
Copy link
Collaborator

Mygod commented Apr 25, 2021

What app? I do not assume any app relies on ping these days.

@RPRX
Copy link
Author

RPRX commented Apr 25, 2021

@Mygod 比如 https://t.me/projectXray/424048

此外,实测 Netch 是本地响应 ping,@Dreamacro 说 Clash 也是本地响应 ping,我认为他们的做法具有参考意义,毕竟 ping 全丢是有些奇怪的事情,加个本地响应并不多余,可以减少一些非预期情况、解决一些潜在的问题

我使用的是 XTLS 的版本,这是一个礼貌性 PR,看你们是否需要了

tun2socks/tun2socks.c Outdated Show resolved Hide resolved
tun2socks/tun2socks.c Outdated Show resolved Hide resolved
tun2socks/tun2socks.c Outdated Show resolved Hide resolved
tun2socks/tun2socks.c Outdated Show resolved Hide resolved
@RPRX
Copy link
Author

RPRX commented Apr 26, 2021

@madeye

根据原有代码,添加了 #define IPV4_PROTOCOL_ICMP 1#define IPV6_NEXT_ICMP 58;ping 和 ping6 那两段需要保持 hard code,因为它们与其 response 的差值决定了响应的 checksum 的快速算法,需要体现出来,且只有这里的代码在解析 ICMP,原有代码对这种情况也是 hard code

@Mygod
Copy link
Collaborator

Mygod commented Apr 26, 2021

If I understand correctly, does this respond to every single ping packet to every IP address? This is very confusing behavior and possibly a fingerprint for the VPN.

It is common for firewalls to block all ping traffic so no modern apps should ever expect ping to work.

@RPRX
Copy link
Author

RPRX commented Apr 26, 2021

@Mygod

Well, my points are already clear above.

  1. And I don't see this behavior a fingerprint for the VPN, because it's not hard for an APP to find itself being proxied.
  2. It's definitely not common for firewalls to block all ping traffic. And you can't define what modern apps should do.
  3. But indeed we can make apps believe target's accessible whenever they want to ping, it's an access tool, after all.

It makes me feel better to see ping's available in Termux when I use v2rayNG, and it's beautiful, rather than no response. I believe Clash and Netch feel the same. So if you don't need this PR, you can just close it. At last, I respect @madeye's decision.

@RPRX RPRX closed this Apr 30, 2021
@RPRX RPRX deleted the main branch April 30, 2021 09:27
@RPRX RPRX restored the main branch April 30, 2021 09:28
@RPRX
Copy link
Author

RPRX commented Apr 30, 2021

刚刚尝试将 main 分支改名为 ping 以专用于此 PR,结果不行,麻烦尽快作出决定,因为若 main 有新的 commit 也会乱入这个 PR

@RPRX RPRX reopened this Apr 30, 2021
@madeye
Copy link

madeye commented Apr 30, 2021

You can close this PR and open a new one with a new branch called ping

@RPRX
Copy link
Author

RPRX commented Apr 30, 2021

@madeye Thanks for your advice, I'll keep this PR as is for now.

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.

6 participants