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

SplitHTTP: Read and validate HTTP/1.1 responses #3797

Merged
merged 5 commits into from
Sep 16, 2024

Conversation

4nd3r5on
Copy link
Contributor

Changes made to read responses from a server
(and check the response codes) before making a request if HTTPv1 is used

Changes made to read responses from a server
(and check the response codes) before making a request if HTTPv1 is used
"time"
)

var (
ErrBadRespCode = errors.New("bad response code")
BadCodes = map[int]struct{}{502: {}, 503: {}, 505: {}}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

everything but 200 OK is bad, can simplify this (also inline the ErrBadRespCode, it carries no type information anyway)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to have some "OK-like" codes there, like 100, 201, 202, and etc codes that may not require outputing the error? I wasn't sure about this so decided to make "bad codes" list on which we definitely need to return the error

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the server is xray, so we can define that 200 is the only valid status code here (you can check hub.go to see that this is the only successful status code in practice)

// ConnHolder implements the net.Conn interface
// adds logic of reading the responses before writing the next request
// Used as a bugfix for HTTP1.1
type ConnHolder struct {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this thing is speciifc to h1 i suggest moving it into yet another file h1_connection (and renaming the constructor + type)

// Used as a bugfix for HTTP1.1
type ConnHolder struct {
ResponsesToRead int
Conn net.Conn
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Conn net.Conn
net.Conn

if you do this then you don't need to forward all the methods


// Optimised to read only response codes
// Reads response codes until getting EOF or error
func ConnHttpReadRespCodes(conn net.Conn) (codes []int, err error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on second look, i think this will actually hang if the connection has not yet produced new responses, which is bad since it tries to read infinite amount of responses. I suggest to read one http response with net/http everytime there is a new request, and that's it.

@RPRX
Copy link
Member

RPRX commented Sep 15, 2024

这个 PR 等 @mmmray 的 approve

SendUploadRequest reads responses, ConnHolder removed
@mmmray
Copy link
Collaborator

mmmray commented Sep 15, 2024

@RPRX pairing with him on tg, I'll approve it when ready

@RPRX
Copy link
Member

RPRX commented Sep 15, 2024

@RPRX pairing with him on tg, I'll approve it when ready

希望我们明天能发新版

@mmmray
Copy link
Collaborator

mmmray commented Sep 15, 2024

I think the deadline for tomorrow cannot be met with this and the UDP noise PR #3794 (I thought the idea of this new versioning was to get rid of deadlines and too much planning...)

I also think neither PR is critical to release

@Fangliding
Copy link
Member

Fangliding commented Sep 15, 2024

Relax, it can be delayed by a day or two. We don't have deadline, just a expected new release

To prevent losing data due to some reader errors like EOF
and to optimise the code
Copy link
Collaborator

@mmmray mmmray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this PR can be merged, except for one nitpick about some code comment

Comment on lines 10 to 11
// To reuse response reader, so we won't lose data
// If some EOF will accure
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the reason you want to reuse the buf reader is because it sometimes reads more data than it returns, and returns the over-read data on the next Read call. so it's not related to EOF

@RPRX
Copy link
Member

RPRX commented Sep 16, 2024

@mmmray 这个 pr 改个合适的标题

@mmmray mmmray changed the title SplitHTTP HTTPv1 fix SplitHTTP: Read and validate HTTP/1.1 responses Sep 16, 2024
@RPRX RPRX merged commit a931507 into XTLS:main Sep 16, 2024
36 checks passed
@4nd3r5on 4nd3r5on deleted the splithttp-http1-fix branch September 16, 2024 12:43
@RPRX
Copy link
Member

RPRX commented Sep 17, 2024

好想给这个 pr 重命名为 SplitHTTP client,强迫症发作了

leninalive pushed a commit to amnezia-vpn/amnezia-xray-core that referenced this pull request Oct 29, 2024
dragonbreath2000 pushed a commit to dragonbreath2000/Xray-core that referenced this pull request Dec 11, 2024
dragonbreath2000 pushed a commit to dragonbreath2000/Xray-core that referenced this pull request Dec 12, 2024
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.

4 participants