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

Wrap slice::from_raw_parts to be compatible with rcl #419

Merged
merged 2 commits into from
Oct 3, 2024

Conversation

mxgrey
Copy link
Collaborator

@mxgrey mxgrey commented Oct 3, 2024

Similar to #416, there are numerous places within rclrs that are at risk of runtime panics because std::slice::from_raw_parts may panic when it receives null pointer values. In particular I got this panic while running cargo test on rclrs:

unsafe precondition(s) violated: slice::from_raw_parts requires the pointer to be aligned and non-null, and the total size of the slice not to exceed `isize::MAX`
stack backtrace:
   0: rust_begin_unwind
             at /rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/std/src/panicking.rs:652:5
   1: core::panicking::panic_nounwind_fmt::runtime
             at /rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/core/src/panicking.rs:110:18
   2: core::panicking::panic_nounwind_fmt
             at /rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/core/src/panicking.rs:120:5
   3: core::panicking::panic_nounwind
             at /rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/core/src/panicking.rs:219:5
   4: core::slice::raw::from_raw_parts::precondition_check
             at /rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/core/src/ub_checks.rs:68:21
   5: core::slice::raw::from_raw_parts
             at /rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/core/src/ub_checks.rs:75:17
   6: rclrs::node::graph::convert_names_and_types
             at ./src/node/graph.rs:425:9
   7: rclrs::node::graph::<impl rclrs::node::Node>::get_names_and_types_by_node
             at ./src/node/graph.rs:345:12
   8: rclrs::node::graph::<impl rclrs::node::Node>::get_publisher_names_and_types_by_node
             at ./src/node/graph.rs:86:9
   9: rclrs::node::graph::tests::test_graph_empty
             at ./src/node/graph.rs:493:32
  10: rclrs::node::graph::tests::test_graph_empty::{{closure}}
             at ./src/node/graph.rs:462:26
  11: core::ops::function::FnOnce::call_once
             at /rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/core/src/ops/function.rs:250:5
  12: core::ops::function::FnOnce::call_once
             at /rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
thread caused non-unwinding panic. aborting.

This PR adds a new internal function rcl_from_raw_parts which wraps std::slice::from_raw_parts so that it's compatible with rcl's practice of returning null pointers to represent empty arrays. Then we replace every call to std::slice::from_raw_parts with rcl_from_raw_parts.

Signed-off-by: Michael X. Grey <[email protected]>
@maspe36
Copy link
Collaborator

maspe36 commented Oct 3, 2024

Awesome, thank you @mxgrey

@maspe36
Copy link
Collaborator

maspe36 commented Oct 3, 2024

You said you saw this when running rclrs test, but how was this not failing in CI?

Copy link
Collaborator

@jhdcs jhdcs left a comment

Choose a reason for hiding this comment

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

These changes look good to me! Good catch!

@mxgrey
Copy link
Collaborator Author

mxgrey commented Oct 3, 2024

You said you saw this when running rclrs test, but how was this not failing in CI?

If I understand correctly, the panic was only put in starting around Rust version 1.78. I think the version of Rust on my own computer is 1.80, but we might only be using 1.74 in the CI. Maybe if we bumped the version in CI we'd see the failure. I can try to create a bogus PR to test that hypothesis.

@mxgrey
Copy link
Collaborator Author

mxgrey commented Oct 3, 2024

The hypothesis held true. Would you like me to merge #420 into this PR or should we merge this PR first and then see that it resolves #420?

@esteve
Copy link
Collaborator

esteve commented Oct 3, 2024

@mxgrey I prefer merging this PR first, as for #420 I'd rather have an explicit version of the Rust toolchain there (instead of stable), I think it makes it easier to track what version of Rust we support

Copy link
Collaborator

@esteve esteve left a comment

Choose a reason for hiding this comment

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

@mxgrey good catch!

@esteve esteve merged commit ad9667f into ros2-rust:main Oct 3, 2024
3 checks passed
@mxgrey
Copy link
Collaborator Author

mxgrey commented Oct 3, 2024

I think it makes it easier to track what version of Rust we support

Makes sense. For the Rust projects that I manage, I use the latest stable version in CI and have the CI run for the main branch once a week, even if there have been no changes. That has allowed me to catch things that have broken due to upstream compiler changes right away.

But targeting a specific version can reduce churn, so it's just a trade-off in priorities.

@mxgrey mxgrey deleted the rcl_from_raw_parts branch October 3, 2024 15:54
@esteve
Copy link
Collaborator

esteve commented Oct 3, 2024

have the CI run for the main branch once a week, even if there have been no changes.

Thats a great idea, we should do that here as well, with the latest stable Rust toolchain to catch any issues with the compiler

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 this pull request may close these issues.

4 participants