Skip to content

tls.Client: Reduce temporary buffer size for reads to prevent buffer overflow. #16436

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

Closed
wants to merge 1 commit into from

Conversation

hawkbee
Copy link
Contributor

@hawkbee hawkbee commented Jul 18, 2023

Resolves #15226, #15590.

@matu3ba
Copy link
Contributor

matu3ba commented Jul 18, 2023

Suggestions:

  • Use as title tls.Client: Reduce temporary buffer size for reads to prevent buffer overflow.
  • Explain how this solves the issues

@hawkbee hawkbee changed the title tls.Client: Reduce temporary buffer size for read in case of buffer overflow. tls.Client: Reduce temporary buffer size for reads to prevent buffer overflow. Jul 18, 2023
@hawkbee
Copy link
Contributor Author

hawkbee commented Jul 18, 2023

Suggestions:

  • Use as title tls.Client: Reduce temporary buffer size for reads to prevent buffer overflow.

ok.

  • Explain how this solves the issues

I am not understand the code fully, When i test with another pull request #16114, my https test nearly panic ervery time.
I try to understand the TLS read flow, and see the buffer is 4 times of the TLS client buffer. If the underlying read return too many data, data copy may overflow, i guess.

After this change, no panic again, i had run test 200 times, all https download success.

This may be not the right way to solve the problem, but i had try my best to get here, and give others a hint to solve this problem.

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

Please explain why you believe this change to solve the problem, and what kinds of tests you performed to validate this change.

Edit: sorry I see that you did that above, in response to @matu3ba. I'm sorry but that is not a satisfactory answer. I will close this PR so that we can do more robust engineering via #14171 and use them to test each other.

@andrewrk andrewrk closed this Oct 19, 2023
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.

Sporadic 'index out of bounds' panic in tls client
3 participants