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

Mixed inbound: Handle immediately closing connection gracefully #4297

Merged
merged 2 commits into from
Jan 17, 2025

Conversation

rPDmYQ
Copy link
Contributor

@rPDmYQ rPDmYQ commented Jan 16, 2025

There's an annoying (and scary) log when socks inbound received a connection that immediately closes (e.g. v2rayN probes socks port to see if the core is ready):

[Warning] [1650329060] app/proxyman/inbound: connection ends > proxy/http: failed to read http request > malformed HTTP request "\x00"

Recognize this case, print a correctly worded Debug level log instead of the Warning with scary message.

@Fangliding
Copy link
Member

Fangliding commented Jan 16, 2025

I think it should not in debug level, user should get some warning info if the request is invalid
or just pass it to socks handler, just like it used to be

@rPDmYQ
Copy link
Contributor Author

rPDmYQ commented Jan 16, 2025

just pass it to socks handler, just like it used to be

That message is confusing, especially when diagnosing connectivity issues, so at least I'd like it to explain the situation better.

the request is invalid

I honestly never saw other programs complaining about a immediately closing connection. Maybe we can make it Info to inform the user, but Warning feels a bit too high for me, given that this happens so common.

By the way, it's definitely a bug that even if conn.Read(firstbyte) received nothing, the unfilled buffer is still being parsed. So instead of malformed HTTP request "\x00" at least it should report unexpected EOF.

@Fangliding
Copy link
Member

I know the handling of the firstbyte is incorrect and should be fixed
About the log, my thought is the behavior of all inbound should be consistent.
If we do this here, should we ignore such requests in http inbound, or all of inbounds?
In other outbounds, it does indeed output at the info level now, so I think the info level is appropriate. This is an old problem, any err return value is passed layer by layer in goalng and finally output at the info level.
So pass the conn to socks handler and let it return EOF log is correct

@RPRX
Copy link
Member

RPRX commented Jan 17, 2025

这是个挺好的修复,改成 info 吧,下面那个是 debug 是因为太频繁了,只是记录一下

并且我觉得 Xray 的日志挺混乱的,有些地方该报 info 而不是 warning,你有发现的可以再提交 PR 给改掉

@RPRX RPRX changed the title Socks inbound: Handle immediately closing connection gracefully Mixed inbound: Handle immediately closing connection gracefully Jan 17, 2025
@RPRX RPRX merged commit 30cb22a into XTLS:main Jan 17, 2025
35 checks passed
@RPRX
Copy link
Member

RPRX commented Jan 17, 2025

感谢 PR,这些代码与 mixed 强相关,标题改为 Mixed inbound 了

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