search

rust-osdev / linked-list-allocator

Apache License 2.0
219 stars 53 forks source link

Fix free logic #64

Closed jamesmunns closed 2 years ago

jamesmunns commented 2 years ago

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 commented 2 years ago

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.

haraldh commented 2 years ago

@phil-opp please release 0.10.1 with these fixes

phil-opp commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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