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 free logic #64

Merged
merged 3 commits into from Jul 7, 2022
Merged

Conversation

jamesmunns
Copy link
Collaborator

The existing 0.10.0 free logic can be incorrect in certain conditions. This can lead to failing assertions due to incorrect preconditions.

I plan to add tests to this to ensure that there are no additional issues, but this should likely be merged ASAP and 0.10.0 should be yanked.

@jamesmunns
Copy link
Collaborator Author

This fixes two bugs in the 0.10.0 release.

  1. The insertion logic in the prior version was incorrect, throwing false-positive assertions
  2. Additionally, there were some cases when a freed allocation was at the start/end of the allocation region, but there was not room for a hole before/after the allocation, essentially leading to leaking of that memory

In order to test this, I've added a much more exhaustive test, which allocations and frees in a variety of different size, alignment, and free-order strategies.

@jamesmunns jamesmunns marked this pull request as ready for review July 2, 2022 01:06
@haraldh
Copy link
Contributor

haraldh commented Jul 6, 2022

@phil-opp please release 0.10.1 with these fixes

@phil-opp
Copy link
Member

phil-opp commented Jul 6, 2022

Thanks for the ping and sorry for the delay! I yanked v0.10.0 on crates.io and I will review this PR as soon as possible. Does this PR supersede your PR at #63, or should both be merged?

@jamesmunns
Copy link
Collaborator Author

I think this change supercedes #63, though if you'd like me to merge in @haraldh's test case changes, I can!

@phil-opp if you'd like to add me as a maintainer for this crate and share crates-io permissions, I'm happy to help out.

Sorry again for the fire drill!

@phil-opp
Copy link
Member

phil-opp commented Jul 6, 2022

No worries, thanks to both of you for creating fixes so quickly!

@phil-opp if you'd like to add me as a maintainer for this crate and share crates-io permissions, I'm happy to help out.

That would be great! I just set up a team for maintaining this crate and sent you an invite. You should then have crates.io access too.

@jamesmunns
Copy link
Collaborator Author

Got it! Let me know if you'd like me to merge this + release, otherwise I'll hold on for your review!

Copy link
Member

@phil-opp phil-opp left a comment

Choose a reason for hiding this comment

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

I didn't have time for an in-depth review, but the changes look all reasonable. Thanks again and sorry for the delay!

Feel free to merge and release this yourself, then we see if I set up the permissions correctly. For creating the releases of this crate, I use the cargo-release tool, which will automatically bump the version number, create the release tags, and update the changelog headings. Just cargo release patch --execute should be enough.

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.

None yet

3 participants