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

Make FrameAllocator an unsafe trait? #69

Closed
phil-opp opened this issue Apr 29, 2019 · 0 comments · Fixed by #71
Closed

Make FrameAllocator an unsafe trait? #69

phil-opp opened this issue Apr 29, 2019 · 0 comments · Fixed by #71

Comments

@phil-opp
Copy link
Member

This issue proposes to make the FrameAllocator trait unsafe to implement. This would allow functions to trust FrameAllocator parameters.

Currently it is safe to implement the FrameAllocator trait for a type. So it's entirely possible to safely construct a type that yields the same frame over and over again, even though it is not unused anymore. For this reason, all functions that rely on FrameAllocator parameter should be unsafe because the caller has to guarantee that the passed frame allocator is implemented correctly. If we make the FrameAllocator trait an unsafe trait, we can rule out invalid frame allocators.

This is similar to the Alloc trait of the standard library. Instead of making it safe to implement the trait and mistrusting all Alloc instances, the trait is unsafe so that it can be used in types/functions without making all (constructor) functions unsafe.

This would be a breaking change, but I think that it is worth it. What do you think?

Example

With the current FrameAllocator trait, we have to write code like this:

pub trait FrameAllocator<S: PageSize> {
    fn allocate_frame(&mut self) -> Option<PhysFrame<S>>;
}

unsafe fn create_mapping(
    mapper: &mut impl Mapper,
    frame_allocator: &mut impl FrameAllocator
) {
   let frame = frame_allocator.allocate_frame();
   mapper.map_to(some_page, frame, some_flags, frame_allocator);
}

We can't trust the frame_allocator, because it is possible to create invalid frame allocators in safe code. Therefore, this must be unsafe because the caller has to guarantee that the frame allocator is valid.

If we make the FrameAllocator trait unsafe to implement, the create_mapping function can be safe:

pub unsafe trait FrameAllocator<S: PageSize> {
    fn allocate_frame(&mut self) -> Option<PhysFrame<S>>;
}

fn create_mapping(
    mapper: &mut impl Mapper,
    frame_allocator: &mut impl FrameAllocator
) {
    []
}

Now we know the frame_allocator can be only invalid if somebody messed up inside an unsafe block (when implementing the trait).

bors bot added a commit that referenced this issue May 2, 2019
71: Make FrameAllocator an unsafe trait r=phil-opp a=phil-opp

This is a breaking change.

Fixes #69 

Co-authored-by: Philipp Oppermann <[email protected]>
@bors bors bot closed this as completed in #71 May 2, 2019
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 a pull request may close this issue.

1 participant