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

grow_to function assumes final free range is at the end of the overall range #1

Open
johnnyrockett opened this issue May 5, 2024 · 5 comments

Comments

@johnnyrockett
Copy link

I ran into crashes when freeing ranges and tracked it down to a bug in the grow_to function.

pub fn grow_to(&mut self, new_end: T) {
    if let Some(last_range) = self.free_ranges.last_mut() {
        last_range.end = new_end;
    } else {
        self.free_ranges.push(self.initial_range.end..new_end);
    }

    self.initial_range.end = new_end;
}

If there is still a free range of memory, this code assumes that it is at the end of the overall range and edits its end to be the new grown end. However, if the last span of freed memory isn't at the very end of the overall range, this will effectively free all the allocated memory in between the last freed span of memory and the end of the overall range.

Something like this change would fix the bug:

pub fn grow_to(&mut self, new_end: T) {
    if let Some(last_range) = self.free_ranges.last_mut() {
        if last_range.end == self.initial_range.end {
            last_range.end = new_end;
        } else {
            self.free_ranges.push(self.initial_range.end..new_end);
        }
    } else {
        self.free_ranges.push(self.initial_range.end..new_end);
    }

    self.initial_range.end = new_end;
}
@cwfitzgerald
Copy link
Member

I might actually be hitting this too - would you be willing to submit a PR, possibly with a test to make sure this doesn't come back?

@dbartussek
Copy link
Contributor

I just ran into the exact same issue, could have saved some time by looking here first

I've created a pull request with essentially the fix proposed by @johnnyrockett (just replacing the second if with same body by using Option::filter) and two test cases for a hole at the beginning and a hole in the middle of allocations: #2

@John-Nagle
Copy link

Ah. Could this be the cause of

BVE-Reborn/rend3#546

@John-Nagle
Copy link

I've built Rend3 and my Sharpview metaverse viewer using this fix. Overrode range-alloc in crates.io with trunk from github. So far, this seems to have fixed BVE-Reborn/rend3#546 I've been running a test for eight hours now without a failure.

@John-Nagle
Copy link

This looks good. Please bump version number and push to crates.io. Thanks.

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

4 participants