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

Conversion from/to raw handle #57

Merged
merged 6 commits into from
Mar 28, 2023
Merged

Conversion from/to raw handle #57

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c1b7be2
Add methods to get raw handle back
agerasev Mar 16, 2023
7253162
Remove unsafe mutex deconstruction
agerasev Mar 21, 2023
bccf041
Mutex conversion from/to inner parts and raw handle
agerasev Mar 21, 2023
7cc3345
Add safety doc comment to mutex `from_raw_handle`
agerasev Mar 22, 2023
ed43701
Remove 'unsafe' where it is not necessary
agerasev Mar 24, 2023
0df4f17
Add safety section to all public unsafe functions
agerasev Mar 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Add safety section to all public unsafe functions
  • Loading branch information
@agerasev
agerasev committed Mar 24, 2023
commit 0df4f1751f6ab17e36cf064173048df2bc8f89eb
2 changes: 2 additions & 0 deletions freertos-rust/src/mutex.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,8 @@ where

/// # Safety
///
/// `handle` must be a valid FreeRTOS mutex handle.
///
/// The type of `handle` (normal or recursive mutex) must match the type
/// of instance being created ([`MutexNormal`] or [`MutexRecursive`] respectively).
unsafe fn from_raw_handle(handle: FreeRtosSemaphoreHandle) -> Self;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a risk that the wrong type of handle is passed in here and later treated incorrectly? E.g. if MutexRecursive::from_raw_handle() is called with a handle made via xSemaphoreCreateBinary(), what happens when that MutexRecursive instance is used to give/take? The docs for xSemaphoreTakeRecursive() say this, which sounds like it would be a problem:
The mutex must have previously been created using a call to xSemaphoreCreateRecursiveMutex();
I also see the CreateRecursiveMutex docs say:
xSemaphoreTake() and xSemaphoreGive() must not be used.

It's not necessarily a deal breaker, but it would be nice to avoid such errors and confusion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Links to docs I mentioned:
https://www.freertos.org/xSemaphoreTakeRecursive.html
https://www.freertos.org/xSemaphoreCreateRecursiveMutex.html

Also occurs to me that this could cause issues if the user leaks the handle with raw_handle() and then drops the owning struct, leaving the handle invalid behind.

What are you envisioning is the use for the user accessing the raw handle like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a risk that the wrong type of handle is passed in here and later treated incorrectly?

Yes, such risk exists as well as risk of passing a null or dangling pointer. This method is unsafe and that usually means that it's up to user to ensure that the handle passed is valid and of correct type.
I didn't find a way to check the mutex type in FreeRTOS API. It seems, that the only thing we can do here is to write a doc comment to draw the user's attention to possible mistakes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also occurs to me that this could cause issues if the user leaks the handle with raw_handle() and then drops the owning struct, leaving the handle invalid behind.

Yes, such issue is also possible. But the raw handle is just a pointer and it's safe in Rust to have dangling pointers. And if you want to use that raw handle you need to call from_raw_handle which is unsafe that means you have to make sure that the raw handle is valid.

Copy link
Contributor Author

@agerasev agerasev Mar 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

What are you envisioning is the use for the user accessing the raw handle like this?

In my case I just needed to get a task identifier which is unique among existing tasks. And for completeness I've added the raw_handle method to all types that already have from_raw_handle and also added raw handle conversion for mutexes.

It's common practice for safe Rust wrappers of foreign API to provide an access to underlying unsafe entities (for example, raw fd traits in Rust stdlib). This can be useful when you want to interoperate with foreign code or use some API functions that aren't yet supported by the wrapper.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fair enough. I see you already added a comment here but any other new unsafe functions should have comments (this always seemed like a good guide to me for reference)

Expand Down
5 changes: 5 additions & 0 deletions freertos-rust/src/queue.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@ impl<T: Sized + Copy> Queue<T> {
})
}

/// # Safety
///
/// `handle` must be a valid FreeRTOS regular queue handle (not semaphore or mutex).
///
/// The item size of the queue must match the size of `T`.
#[inline]
pub unsafe fn from_raw_handle(handle: FreeRtosQueueHandle) -> Self {
Self {
Expand Down
6 changes: 6 additions & 0 deletions freertos-rust/src/semaphore.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,12 @@ impl Semaphore {
}
}

/// # Safety
///
/// `handle` must be a valid FreeRTOS semaphore handle.
///
/// Only binary or counting semaphore is expected here.
/// To create mutex from raw handle use [`crate::mutex::MutexInnerImpl::from_raw_handle`].
#[inline]
pub unsafe fn from_raw_handle(handle: FreeRtosSemaphoreHandle) -> Self {
Self { semaphore: handle }
Expand Down
3 changes: 3 additions & 0 deletions freertos-rust/src/task.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,9 @@ impl Task {
}
}

/// # Safety
///
/// `handle` must be a valid FreeRTOS task handle.
#[inline]
pub unsafe fn from_raw_handle(handle: FreeRtosTaskHandle) -> Self {
Self { task_handle: handle }
Expand Down
5 changes: 5 additions & 0 deletions freertos-rust/src/timers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,11 @@ impl Timer {
}

/// Create a timer from a raw handle.
///
/// # Safety
///
/// `handle` must be a valid FreeRTOS timer handle.
#[inline]
pub unsafe fn from_raw_handle(handle: FreeRtosTimerHandle) -> Self {
Self { handle }
}
Expand Down
3 changes: 3 additions & 0 deletions freertos-rust/src/utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ pub fn shim_sanity_check() -> Result<(), TypeSizeError> {
Ok(())
}

/// # Safety
///
/// `str` must be a pointer to the beginning of nul-terminated sequence of bytes.
#[cfg(any(feature = "time", feature = "hooks", feature = "sync"))]
pub unsafe fn str_from_c_string(str: *const u8) -> Result<String, FreeRtosError> {
let mut buf = Vec::new();
Expand Down