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

Fix improper memory reuse in NewFastHTTPHandler #1860

Merged
merged 5 commits into from
Sep 10, 2024

Conversation

sigmundxia
Copy link
Contributor

Hi Developers,

This pull request fixes the improper memory reuse in NewFastHTTPHandler reported in this issue.

If there are any further adjustments or improvements you'd like me to make, please don't hesitate to reach out :)

Best regards!

header.go Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

case "Transfer-Encoding":
r.TransferEncoding = append(r.TransferEncoding, sv)
default:
r.Header.Set(sk, sv)
}

I think performing deep copying through a case branch would be more appropriate here, as the comment in Request.VisitAll mentions that key and value cannot be referenced after the function f finishes executing. Directly modifying VisitAll may affect its efficiency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree :) I have revised and made a new commit

Copy link
Contributor

Choose a reason for hiding this comment

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

Following your initial approach, it would be better to perform a deep copy only for the cookie header. The other references likely don't have any issues.If a deep copy of the value is needed here, the key should also be copied. It seems that either solution could work; it's just a trade-off between performance and memory safety.From a personal perspective. what do you think? @erikdubbelboer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense, I added a new commit to limit deep copy to the cookie case only. If there are further results on the tradeoff between performance and memory safety, I'll be happy to continue to make changes

Copy link
Contributor

@gaby gaby left a comment

Choose a reason for hiding this comment

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

Looks good to me

@erikdubbelboer erikdubbelboer merged commit 65e989e into valyala:master Sep 10, 2024
15 checks passed
@erikdubbelboer
Copy link
Collaborator

Thanks!

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