-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Store: Fix panic too smaller buffer #7658
Conversation
Could you sign this commit? |
56829dc
to
0d9a924
Compare
Signed-off-by: dominic.qi <dominic.qi@jaco.live>
0d9a924
to
4de3a85
Compare
@saswatamcode signed, but the e2e test failed , i can't see error information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a unit test for this?
Hi, @GiedriusS i haven't thought of what unit tests should be added yet. What do you think should be added? just moved the defer statement to the appropriate place. |
I think a test should call that function twice and in the second call it should get a wrongly sized buffer with this bug, right? |
Signed-off-by: qiyang <dominic.qiyang@gmail.com>
I don't think so; the buffer pool returns a wrongly sized buffer because it has a smaller buffer. Just make sure only the obtained buffer can return to the pool. |
I think a unit test is still good to have to catch such regression next time. |
|
Signed-off-by: Ben Ye <benye@amazon.com>
I will get this merged to fix the bug. Thanks @dominicqi. |
Co-authored-by: dominic.qi <dominic.qi@jaco.live> Co-authored-by: Ben Ye <benye@amazon.com>
Changes
Related issue #6934
I think the problem could be: a buffer that was not obtained from the buffer pool should not be put back into it, as it will lead to buffers with incorrect sizes.