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

NewFastHTTPHandler may cause improper memory reuse #1859

Closed
sigmundxia opened this issue Sep 8, 2024 · 3 comments
Closed

NewFastHTTPHandler may cause improper memory reuse #1859

sigmundxia opened this issue Sep 8, 2024 · 3 comments

Comments

@sigmundxia
Copy link
Contributor

Hi developers,

I encountered a strange problem when using the middleware of fiber, a downstream software that depends on fasthttp. I think this problem may be related to fasthttp.

To better illustrate this problem, I added a test for fasthttp to simulate the process of fiber using middleware. As a result, this test failed and reported an error in line 177:

https://github.com/sigmundxia/fasthttp/blob/a165213f6370f59e5d6414f4f573eb6519c472ca/fasthttpadaptor/adaptor_test.go#L142-L187

As I mentioned in the code comments, this test can pass as long as line 172 is commented out, but line 172 is not related to cookies, which indicates that fasthttp may have experienced improper memory reuse in it.

I think there are two possible directions of the solution:

  1. Fix the improper memory reuse problem in fasthttp. I have basically located the code that causes this problem. If you are sure that this is indeed a bug, I am happy to submit a PR to fix this problem.

  2. Confirm that the way to use middleware in fiber is inappropriate. Since the fiber implementation ignores ResponseWriter and r in the middleware and restarts from ctx, this may be inappropriate. If you are sure that this is the reason for the error, I will try to communicate with the fiber developers.

Thank you for your attention! If you have any questions or would like to discuss further with me, please feel free to contact me.

Best wishes.

@newacorn
Copy link
Contributor

newacorn commented Sep 8, 2024

The issue is related to fasthttp, where shared memory is used when copying request cookies from fasthttp to net/http. Your pull request is welcome.

@sigmundxia
Copy link
Contributor Author

The issue is related to fasthttp, where shared memory is used when copying request cookies from fasthttp to net/http. Your pull request is welcome.

Thanks! I've submitted a pull request that will hopefully fix this.

@sigmundxia
Copy link
Contributor Author

Since the pull request has been merged, I think this issue can be closed :)

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

No branches or pull requests

2 participants