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

Improve ReshapeMultiBuffer #1636

Merged
merged 3 commits into from
Feb 24, 2023
Merged

Improve ReshapeMultiBuffer #1636

merged 3 commits into from
Feb 24, 2023

Conversation

H1JK
Copy link
Member

@H1JK H1JK commented Feb 10, 2023

No description provided.

@RPRX RPRX requested a review from yuhan6665 February 10, 2023 02:06
Copy link
Member

@yuhan6665 yuhan6665 left a comment

Choose a reason for hiding this comment

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

感谢大佬的高效实现

@RPRX
Copy link
Member

RPRX commented Feb 10, 2023

Wait

@RPRX
Copy link
Member

RPRX commented Feb 10, 2023

等我加了 REALITY 的代码再 merge,不然又要 rebase 了

@yuhan6665
Copy link
Member

等我加了 REALITY 的代码再 merge,不然又要 rebase 了

好~
不过没有冲突应该无压力

@RPRX
Copy link
Member

RPRX commented Feb 10, 2023

这个文件还好,前几天已经 rebase 到吐了,不想再来一次

buffer2.Write(b.BytesFrom(index))
b.Release()
if index >= buf.Size-21 {
Copy link
Member

Choose a reason for hiding this comment

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

It seems we don't need this check and we can always do b.Resize(0, index)?

Copy link
Member Author

Choose a reason for hiding this comment

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

@yuhan6665 What if tlsApplicationDataStart appears at the last 21 bytes?

And in the same way, it may also worth to consider the case where the newly created buffer2 still does not meet the 21-byte free requirement. (I'm not sure)

Copy link
Member

Choose a reason for hiding this comment

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

Normally a tls record is 23, 3, 3, len_high, len_low, data, tag(16 bytes). So very unlikely it appear at the last 21 bytes.
Note the current way Vision parse and understand the tls record in a simple way, and is not strictly correct. For instance, there could be a 1/16,777,216 chance that 23, 3, 3 appear in the data itself. Someday maybe we can have a real tls parser..
I think reducing unnecessary buffer ops is good enough for this pr.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, it doesn't matter, I think.

@yuhan6665 yuhan6665 merged commit 267d93f into XTLS:main Feb 24, 2023
@yuhan6665
Copy link
Member

Merged. Thanks again!

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