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

Remove owning_ref to avoid (unused) unsoundness #482

Closed
kevinaboos opened this issue Jan 26, 2022 · 4 comments
Closed

Remove owning_ref to avoid (unused) unsoundness #482

kevinaboos opened this issue Jan 26, 2022 · 4 comments
Assignees
@kevinaboos
Labels
minor

Comments

@kevinaboos
Copy link
Member

See: Kimundi/owning-ref-rs#78

We don't currently use the unsound functions, but it's a good idea to patch this nonetheless (in the root Cargo.toml's [patch] section).
@kevinaboos
Copy link
Member Author

Could also consider replacing owning_ref with https://crates.io/crates/reffers

@kevinaboos
Copy link
Member Author

I think we'll need to replace owning_ref entirely. Good candidates are:

  • self_cell: good but a bit limited
  • ouroboros: huge and perhaps too much build-time overhead
  • reffers: not as well-tested as others

@kevinaboos kevinaboos self-assigned this Oct 5, 2022
@kevinaboos
Copy link
Member Author

Note that all other RustSec advisory database warnings have been addressed. owning_ref is all that remains

@kevinaboos kevinaboos mentioned this issue Oct 13, 2022
Merged
kevinaboos added a commit that referenced this issue Oct 31, 2022
@kevinaboos
Remove usage of owning_ref from the window crate (#671)
243c07a 
* Introduces a `Framebuffer`-specific reference type that holds a
  `MutexGuard` for direct, restricted access to a window's `Framebuffer`.

* In the future, this could be made more generic, but this kind of 
  access pattern is still rare in Theseus.

* Avoids known unsoundness in `owning_ref`; see #482
github-actions bot pushed a commit that referenced this issue Oct 31, 2022
@kevinaboos
Remove usage of owning_ref from the window crate (#671)
205263e 
* Introduces a `Framebuffer`-specific reference type that holds a
  `MutexGuard` for direct, restricted access to a window's `Framebuffer`.

* In the future, this could be made more generic, but this kind of
  access pattern is still rare in Theseus.

* Avoids known unsoundness in `owning_ref`; see #482 243c07a
kevinaboos added a commit that referenced this issue Oct 31, 2022
@kevinaboos
Remove owning_ref from ethernet_smoltcp_device crate (#672)
0bd45b8 
Avoids unsoundness in `owning_ref`; see #482
github-actions bot pushed a commit that referenced this issue Oct 31, 2022
@kevinaboos
Remove owning_ref from ethernet_smoltcp_device crate (#672)
293f0dd 
Avoids unsoundness in `owning_ref`; see #482 0bd45b8
kevinaboos added a commit that referenced this issue Oct 31, 2022
@kevinaboos
Use BorrowedMappedPages to replace `owning_ref (#664)
cd8300d 
* This commit adds 2 new types to replace `owning_ref`, which is unsound (see #482).
  Both types avoid the heap allocation (`Box` wrapper) needed in `owning_ref`, 
  which is possible because a `MappedPages` object refers to a memory location
  that can never move or change after the mapping is initially created.
  * `BorrowedMappedPages`: a persistent unique borrow of a `MappedPages` object
     obtained by calling to `MappedPages::as_type()` or `as_type_mut()`. 
  * `BorrowedSliceMappedPages`: same as above, but for slices, which are 
     obtained by calling `MappedPages::as_slice()` or `as_slice_mut()`.

* Both types accept a type parameter to specify the `Mutability` of the borrow,
  which is `Immutable` by default (for `&T` and `&[T]`), but can also be 
  specified as `Mutable` (for `&mut T` and `&mut [T]`).

* All crates where `owning_ref` was previously used to deref a
  `MappedPages` into one of the above variants of type `T`
  have now been replaced with these borrowed types, e.g.,
  framebuffer, ACPI tables, APIC, networking, etc.
github-actions bot pushed a commit that referenced this issue Oct 31, 2022
@kevinaboos
Use BorrowedMappedPages to replace `owning_ref (#664)
3382349 
* This commit adds 2 new types to replace `owning_ref`, which is unsound (see #482).
  Both types avoid the heap allocation (`Box` wrapper) needed in `owning_ref`,
  which is possible because a `MappedPages` object refers to a memory location
  that can never move or change after the mapping is initially created.
  * `BorrowedMappedPages`: a persistent unique borrow of a `MappedPages` object
     obtained by calling to `MappedPages::as_type()` or `as_type_mut()`.
  * `BorrowedSliceMappedPages`: same as above, but for slices, which are
     obtained by calling `MappedPages::as_slice()` or `as_slice_mut()`.

* Both types accept a type parameter to specify the `Mutability` of the borrow,
  which is `Immutable` by default (for `&T` and `&[T]`), but can also be
  specified as `Mutable` (for `&mut T` and `&mut [T]`).

* All crates where `owning_ref` was previously used to deref a
  `MappedPages` into one of the above variants of type `T`
  have now been replaced with these borrowed types, e.g.,
  framebuffer, ACPI tables, APIC, networking, etc. cd8300d
@kevinaboos
Copy link
Member Author

As an alternative and better solution, we no longer support using owning_ref anywhere in Theseus. All of the crates that use it have now been removed; all that's left is to remove it from various lock implementations that offer compatibility with it by implementing stable_deref_trait (e.g., MutexIrqSafe, MutexSleep and similar).

@kevinaboos kevinaboos changed the title Patch owning_ref to avoid (unused) unsoundness Remove owning_ref to avoid (unused) unsoundness Oct 31, 2022
This was referenced Nov 2, 2022
Merged
Merged
Merged
ouz-a pushed a commit to ouz-a/Theseus that referenced this issue Dec 7, 2022
@kevinaboos @ouz-a
Remove usage of owning_ref from the window crate (theseus-os#671)
a09e0ea 
* Introduces a `Framebuffer`-specific reference type that holds a
  `MutexGuard` for direct, restricted access to a window's `Framebuffer`.

* In the future, this could be made more generic, but this kind of 
  access pattern is still rare in Theseus.

* Avoids known unsoundness in `owning_ref`; see theseus-os#482
ouz-a pushed a commit to ouz-a/Theseus that referenced this issue Dec 7, 2022
@kevinaboos @ouz-a
Remove owning_ref from ethernet_smoltcp_device crate (theseus-os#672
fb7f9c4 
)

Avoids unsoundness in `owning_ref`; see theseus-os#482
ouz-a pushed a commit to ouz-a/Theseus that referenced this issue Dec 7, 2022
@kevinaboos @ouz-a
Use BorrowedMappedPages to replace `owning_ref (theseus-os#664)
9b9a17a 
* This commit adds 2 new types to replace `owning_ref`, which is unsound (see theseus-os#482).
  Both types avoid the heap allocation (`Box` wrapper) needed in `owning_ref`, 
  which is possible because a `MappedPages` object refers to a memory location
  that can never move or change after the mapping is initially created.
  * `BorrowedMappedPages`: a persistent unique borrow of a `MappedPages` object
     obtained by calling to `MappedPages::as_type()` or `as_type_mut()`. 
  * `BorrowedSliceMappedPages`: same as above, but for slices, which are 
     obtained by calling `MappedPages::as_slice()` or `as_slice_mut()`.

* Both types accept a type parameter to specify the `Mutability` of the borrow,
  which is `Immutable` by default (for `&T` and `&[T]`), but can also be 
  specified as `Mutable` (for `&mut T` and `&mut [T]`).

* All crates where `owning_ref` was previously used to deref a
  `MappedPages` into one of the above variants of type `T`
  have now been replaced with these borrowed types, e.g.,
  framebuffer, ACPI tables, APIC, networking, etc.
github-actions bot pushed a commit to kevinaboos/Theseus that referenced this issue Jul 7, 2023
@kevinaboos
Remove owning_ref from debug_info (theseus-os#1002)
f75e488 
* Use new `BorrowedSliceMappedPages` feature that allows it to hold
  an `Arc<MappedPages>` instead of just a plain `MappedPages`.

* Closes theseus-os#482 -- `owning_ref` is unsound. 5440ad1
github-actions bot pushed a commit that referenced this issue Jul 7, 2023
@kevinaboos
Remove owning_ref from debug_info (#1002)
39a1c34 
* Use new `BorrowedSliceMappedPages` feature that allows it to hold
  an `Arc<MappedPages>` instead of just a plain `MappedPages`.

* Closes #482 -- `owning_ref` is unsound. 5440ad1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kevinaboos kevinaboos

Labels
minor
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@kevinaboos