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.

Sign up for GitHub

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

Jump to bottom

Fix an API misuse #168

Merged
merged 2 commits into from Aug 24, 2022
Merged

Fix an API misuse #168

merged 2 commits into from Aug 24, 2022

Conversation

Luni-4
Copy link
Member

@Luni-4 Luni-4 commented Aug 24, 2022 •
edited

This PR fixes an API misuse, BytesMut was reading not yet initialized memory. This also fixes a test on Windows.

@Luni-4 Luni-4 requested a review from lu-zero August 24, 2022 09:10
@coveralls
Copy link

coveralls commented Aug 24, 2022 •
edited

Coverage Status

Coverage decreased (-2.03%) to 62.327% when pulling 5c95bb9 on windows into fc2e0cd on master.

@lu-zero
Copy link
Member

lu-zero commented Aug 24, 2022

The crash is due an API misuse. Bytes was reading not yet initialized memory

lu-zero
lu-zero requested changes Aug 24, 2022
View reviewed changes
Copy link
Member

@lu-zero lu-zero left a comment

Choose a reason for hiding this comment

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

Split the commit in the part that updates bytes and the one that fixes its usage.

The second one should be explicit in stating that the previous code is incorrect in any platform and it is fixing an API misuse.

@Luni-4 Luni-4 changed the title Fix a crash in a test on Windows Fix an API misuse Aug 24, 2022
@Luni-4
Fix an API misuse
5c95bb9 
`BytesMut` was reading not yet initialized memory. This also fixes a test
on Windows.
@Luni-4 Luni-4 requested a review from lu-zero August 24, 2022 09:51
@lu-zero
Copy link
Member

lu-zero commented Aug 24, 2022

Thank you

@Luni-4 Luni-4 merged commit 65d27f0 into master Aug 24, 2022
@Luni-4 Luni-4 deleted the windows branch August 24, 2022 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lu-zero lu-zero lu-zero approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Luni-4 @coveralls @lu-zero