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

Panic occurs when publish sequence types for Rust 1.78 #406

Closed
Oscarchoi opened this issue Jun 12, 2024 · 7 comments · Fixed by #407
Closed

Panic occurs when publish sequence types for Rust 1.78 #406

Oscarchoi opened this issue Jun 12, 2024 · 7 comments · Fixed by #407

Comments

@Oscarchoi
Copy link
Contributor

I found that, in Rust 1.78, when trying to publish data of sequence types, such as std_msgs::msg::UInt8MultiArray or messages utilizing sensor_msgs::msg::PointCloud2, a panic occurs with the message:
unsafe precondition(s) violated: ptr::write_bytes requires that the destination pointer is aligned and non-null.

I tested ros2-rust 0.4.1 on ubuntu 22.04.1 + ros2 humble and checked on ubuntu 20.04.6 + ros2 foxy, too.
The source code I tested and the backtrace message are as follows:

source
use rclrs::{Context, Node};

fn main() {
    let context = Context::new([]).expect("failed to create context");

    let node = Node::new(&context, "rust_ros2_node").expect("failed to create node");

    let publisher = node
        .create_publisher::<std_msgs::msg::UInt8MultiArray>("u8_array", rclrs::QOS_PROFILE_DEFAULT)
        .expect("failed to create publisher");

    let layout = std_msgs::msg::MultiArrayLayout {
        dim: [
            std_msgs::msg::MultiArrayDimension {
                label: String::from("x"),
                size: 1,
                stride: 1,
            },
            std_msgs::msg::MultiArrayDimension {
                label: String::from("y"),
                size: 1,
                stride: 1,
            },
            std_msgs::msg::MultiArrayDimension {
                label: String::from("z"),
                size: 1,
                stride: 1,
            },
        ]
        .to_vec(),
        data_offset: 0,
    };

    let msg = std_msgs::msg::UInt8MultiArray {
        layout,
        data: vec![0u8, 1, 2],
    };
    publisher.publish(msg).expect("failed to publish message");

    println!("Published");
}
backtrace
$ ~/Workspace/rclrs-tutorial/sequence$ cargo run
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.06s
     Running `target/debug/sequence`
thread 'main' panicked at library/core/src/panicking.rs:156:5:
unsafe precondition(s) violated: ptr::write_bytes requires that the destination pointer is aligned and non-null
stack backtrace:
   0: rust_begin_unwind
             at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/std/src/panicking.rs:645:5
   1: core::panicking::panic_nounwind_fmt::runtime
             at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/core/src/panicking.rs:110:18
   2: core::panicking::panic_nounwind_fmt
             at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/core/src/panicking.rs:123:9
   3: core::panicking::panic_nounwind
             at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/core/src/panicking.rs:156:5
   4: core::intrinsics::write_bytes::precondition_check
             at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/core/src/intrinsics.rs:2799:21
   5: core::intrinsics::write_bytes
             at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/core/src/intrinsics.rs:3154:9
   6: rosidl_runtime_rs::sequence::<impl rosidl_runtime_rs::traits::SequenceAlloc for u8>::sequence_init
             at /home/oscarchoi/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rosidl_runtime_rs-0.4.1/src/sequence.rs:512:21
   7: rosidl_runtime_rs::sequence::Sequence<T>::new
             at /home/oscarchoi/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rosidl_runtime_rs-0.4.1/src/sequence.rs:252:13
   8: <rosidl_runtime_rs::sequence::Sequence<T> as core::iter::traits::collect::FromIterator<T>>::from_iter
             at /home/oscarchoi/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rosidl_runtime_rs-0.4.1/src/sequence.rs:202:23
   9: <rosidl_runtime_rs::sequence::Sequence<T> as core::convert::From<alloc::vec::Vec<T>>>::from
             at /home/oscarchoi/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rosidl_runtime_rs-0.4.1/src/sequence.rs:193:9
  10: <T as core::convert::Into<U>>::into
             at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/core/src/convert/mod.rs:759:9
  11: <std_msgs::msg::UInt8MultiArray as rosidl_runtime_rs::traits::Message>::into_rmw_message
             at /home/oscarchoi/ros2_ws/install/std_msgs/share/std_msgs/rust/src/msg.rs:3142:15
  12: rclrs::publisher::Publisher<T>::publish
             at /home/oscarchoi/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rclrs-0.4.1/src/publisher.rs:148:27
  13: sequence::main
             at ./src/main.rs:38:5
  14: core::ops::function::FnOnce::call_once
             at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/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.
Aborted (core dumped)

It seems that the panic arises at the following location:

unsafe {
// This allocates space and sets seq.size and seq.capacity to size
let ret = $init_func(seq as *mut _, size);
// Zero memory, since it will be uninitialized if there is no default value
std::ptr::write_bytes(seq.data, 0u8, size);
ret

In the current implementation, if the size is 0 (which happens when initialization), it's expected that seq.data will be null. However, according to the latest std documentation (https://doc.rust-lang.org/nightly/std/ptr/fn.write_bytes.html), even if the number of bytes being written is 0, the pointer should not be null. I believe this is causing the panic.

It seems that a null pointer is inevitable when the size is 0, due to the implementation of rosidl_runtime (https://github.com/ros2/rosidl/blob/4ba0effa201030ae8f45597b29d4ca685b2d50a1/rosidl_runtime_c/src/primitives_sequence_functions.c#L24-L43).

I'm quite new to rust and not sure what the correct solution is, I found that adding a null check before the write_bytes prevents the panic and makes the publish work. Oscarchoi@8b1e786

@Guelakais
Copy link
Contributor

Why do you use a deprecated Message type? What else did you want to achieve with your publisher?

@Oscarchoi
Copy link
Contributor Author

@Guelakais
Oh, I just picked std_msgs::msg::UInt8MultiArray to provide a simpler example, but it seems it caused you some confusion.

As I mentioned in the text, this issue occurs with messages that use sequence types, such as sensor_msgs::msg::PointCloud2 or sensor_msgs::msg::Joy. They include array types, such as uint8[] data, in the message definition, and the publisher causes a panic when trying to publish these messages.

I developed a publisher for sensor_msgs::msg::PointCloud2 and added an example of it below.

source
use rclrs::{Context, Node};

use sensor_msgs::msg::PointCloud2;
use sensor_msgs::msg::PointField;

fn main() {
    let context = Context::new([]).expect("failed to create node");

    let node =
        Node::new(&context, "rust_ros2_pointcloud_publisher").expect("failed to create node");

    let publisher = node
        .create_publisher::<PointCloud2>("pointcloud", rclrs::QOS_PROFILE_DEFAULT)
        .expect("failed to create publisher");

    let duration_since_epoch = std::time::SystemTime::now()
        .duration_since(std::time::UNIX_EPOCH)
        .unwrap();

    let header = std_msgs::msg::Header {
        stamp: builtin_interfaces::msg::Time {
            sec: duration_since_epoch.as_secs() as i32,
            nanosec: duration_since_epoch.subsec_nanos(),
        },
        frame_id: "map".to_string(),
    };

    // for test
    let data = vec![
        0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, // (0, 0, 0)
        0, 0, 128, 63, 0, 0, 128, 63, 0, 0, 128, 63, // (1.0, 1.0, 1.0)
        0, 0, 0, 64, 0, 0, 0, 64, 0, 0, 0, 64, // (2.0, 2.0, 2.0)
    ];

    let pointcloud = PointCloud2 {
        header: header,
        height: 1,
        width: 3,
        fields: vec![
            PointField {
                name: "x".to_string(),
                offset: 0,
                datatype: PointField::FLOAT32,
                count: 1,
            },
            PointField {
                name: "y".to_string(),
                offset: 4,
                datatype: PointField::FLOAT32,
                count: 1,
            },
            PointField {
                name: "z".to_string(),
                offset: 8,
                datatype: PointField::FLOAT32,
                count: 1,
            },
        ],
        is_bigendian: false,
        point_step: 12,
        row_step: 36,
        data: data,
        is_dense: true,
    };

    publisher
        .publish(pointcloud)
        .expect("failed to publish message");

    println!("Published PointCloud2 message");
}
panic message
$ cargo run --bin pointcloud
   Compiling hello_world v0.1.0 (/home/oscarchoi/Workspace/rclrs-tutorial/hello_world)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.33s
     Running `target/debug/pointcloud`
thread 'main' panicked at library/core/src/panicking.rs:156:5:
unsafe precondition(s) violated: ptr::write_bytes requires that the destination pointer is aligned and non-null
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread caused non-unwinding panic. aborting.
Aborted (core dumped)

@mxgrey
Copy link
Collaborator

mxgrey commented Jun 20, 2024

Thanks for reporting this @Oscarchoi . I think we should start by using the test_msgs package to write tests that cover every kind of message field that might be sent, including second-order fields (e.g. nested messages and arrays of messages).

In the best case scenario we would have a test that runs two processes and sends every test message type from one process to the other.

@Oscarchoi
Copy link
Contributor Author

@mxgrey Thanks! That would be very helpful. If there are any issues or plans that arise later, I'd like to help as well :)

And if you confirm that the changes I made are appropriate, I'll be happy to submit a PR for the fix.
Oscarchoi@8b1e786

@Guelakais
Copy link
Contributor

Thank you for the detailed explanation. ros2 rust is a ros2 interface written in rust and I'm not sure with many bugs if they are generated by rust or directly by ros2. In this case it seems to be clear. I'll play around with it a bit soon. If you are generally interested in what you can do with ros2 rust, I can recommend a few sources:

@jhdcs
Copy link
Collaborator

jhdcs commented Jun 20, 2024

@mxgrey Thanks! That would be very helpful. If there are any issues or plans that arise later, I'd like to help as well :)

And if you confirm that the changes I made are appropriate, I'll be happy to submit a PR for the fix. Oscarchoi@8b1e786

You can feel free to submit the PR as-is. Part of the PR process is determining if changes are appropriate or not.

At first glance, though, things look fine to me, though a few extra pairs of eyes on the code would be appreciated. There might be some sort of possible consequence that I haven't considered, after all.

In any case, thank you not only for the report but for also looking into this! It's much appreciated!

@Oscarchoi
Copy link
Contributor Author

Thank you for the detailed explanation. ros2 rust is a ros2 interface written in rust and I'm not sure with many bugs if they are generated by rust or directly by ros2. In this case it seems to be clear. I'll play around with it a bit soon. If you are generally interested in what you can do with ros2 rust, I can recommend a few sources:

Thank you for the advice! I will use it as a reference for utilizing rust_ros2 :)

You can feel free to submit the PR as-is. Part of the PR process is determining if changes are appropriate or not.

At first glance, though, things look fine to me, though a few extra pairs of eyes on the code would be appreciated. There might be some sort of possible consequence that I haven't considered, after all.

In any case, thank you not only for the report but for also looking into this! It's much appreciated!

Thank you for reviewing it. And I'm glad to hear that it was helpful :)

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.

4 participants