Skip to content

Make rclrs panic-free as far as possible #190

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

Merged
merged 1 commit into from
Jun 6, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 7 additions & 5 deletions rclrs/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,16 +59,18 @@ impl Context {
/// let invalid_remapping = ["--ros-args", "-r", ":=:*/]"].map(String::from);
/// assert!(Context::new(invalid_remapping).is_err());
/// ```
///
/// # Panics
/// When there is an interior null byte in any of the args.
pub fn new(args: impl IntoIterator<Item = String>) -> Result<Self, RclrsError> {
// SAFETY: Getting a zero-initialized value is always safe
let mut rcl_context = unsafe { rcl_get_zero_initialized_context() };
let cstring_args: Vec<CString> = args
.into_iter()
.map(|arg| CString::new(arg).unwrap())
.collect();
.map(|arg| {
CString::new(arg.as_str()).map_err(|err| RclrsError::StringContainsNul {
err,
s: arg.clone(),
})
})
.collect::<Result<_, _>>()?;
// Vector of pointers into cstring_args
let c_args: Vec<*const c_char> = cstring_args.iter().map(|arg| arg.as_ptr()).collect();
unsafe {
Expand Down
22 changes: 16 additions & 6 deletions rclrs/src/error.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::rcl_bindings::*;
use std::error::Error;
use std::ffi::CStr;
use std::ffi::{CStr, NulError};
use std::fmt::{self, Display};

/// The main error type.
Expand All @@ -20,13 +20,23 @@ pub enum RclrsError {
/// The error message set in the `rcl` layer or below.
msg: Option<RclErrorMsg>,
},
/// A string provided to `rclrs` could not be converted into a `CString`.
StringContainsNul {
/// The string that contains a nul byte.
s: String,
/// The error indicating the position of the nul byte.
err: NulError,
},
}

impl Display for RclrsError {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match self {
RclrsError::RclError { code, .. } => write!(f, "{}", code),
RclrsError::UnknownRclError { code, .. } => write!(f, "{}", code),
RclrsError::StringContainsNul { s, .. } => {
write!(f, "Could not convert string '{}' to CString", s)
}
}
}
}
Expand Down Expand Up @@ -54,11 +64,11 @@ impl Error for RclErrorMsg {}

impl Error for RclrsError {
fn source(&self) -> Option<&(dyn Error + 'static)> {
let msg = match self {
RclrsError::RclError { msg, .. } => msg,
RclrsError::UnknownRclError { msg, .. } => msg,
};
msg.as_ref().map(|e| e as &dyn Error)
match self {
RclrsError::RclError { msg, .. } => msg.as_ref().map(|e| e as &dyn Error),
RclrsError::UnknownRclError { msg, .. } => msg.as_ref().map(|e| e as &dyn Error),
RclrsError::StringContainsNul { err, .. } => Some(err).map(|e| e as &dyn Error),
}
}
}

Expand Down
15 changes: 10 additions & 5 deletions rclrs/src/node/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,13 +148,18 @@ impl NodeBuilder {
///
/// For example usage, see the [`NodeBuilder`][1] docs.
///
/// # Panics
/// When the node name or namespace contain null bytes.
///
/// [1]: crate::NodeBuilder
pub fn build(&self) -> Result<Node, RclrsError> {
let node_name = CString::new(self.name.as_str()).unwrap();
let node_namespace = CString::new(self.namespace.as_str()).unwrap();
let node_name =
CString::new(self.name.as_str()).map_err(|err| RclrsError::StringContainsNul {
err,
s: self.name.clone(),
})?;
let node_namespace =
CString::new(self.namespace.as_str()).map_err(|err| RclrsError::StringContainsNul {
err,
s: self.namespace.clone(),
})?;

// SAFETY: No preconditions for this function.
let mut node_handle = unsafe { rcl_get_zero_initialized_node() };
Expand Down
8 changes: 4 additions & 4 deletions rclrs/src/node/publisher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,6 @@ where
/// Creates a new `Publisher`.
///
/// Node and namespace changes are always applied _before_ topic remapping.
///
/// # Panics
/// When the topic contains interior null bytes.
pub fn new(node: &Node, topic: &str, qos: QoSProfile) -> Result<Self, RclrsError>
where
T: Message,
Expand All @@ -74,7 +71,10 @@ where
let mut publisher_handle = unsafe { rcl_get_zero_initialized_publisher() };
let type_support =
<T as Message>::RmwMsg::get_type_support() as *const rosidl_message_type_support_t;
let topic_c_string = CString::new(topic).unwrap();
let topic_c_string = CString::new(topic).map_err(|err| RclrsError::StringContainsNul {
err,
s: topic.into(),
})?;
let node_handle = &mut *node.handle.lock();

// SAFETY: No preconditions for this function.
Expand Down
8 changes: 4 additions & 4 deletions rclrs/src/node/subscription.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,6 @@ where
T: Message,
{
/// Creates a new subscription.
///
/// # Panics
/// When the topic contains interior null bytes.
pub fn new<F>(
node: &Node,
topic: &str,
Expand All @@ -91,7 +88,10 @@ where
let mut subscription_handle = unsafe { rcl_get_zero_initialized_subscription() };
let type_support =
<T as Message>::RmwMsg::get_type_support() as *const rosidl_message_type_support_t;
let topic_c_string = CString::new(topic).unwrap();
let topic_c_string = CString::new(topic).map_err(|err| RclrsError::StringContainsNul {
err,
s: topic.into(),
})?;
let node_handle = &mut *node.handle.lock();

// SAFETY: No preconditions for this function.
Expand Down