Merge pull request zivid#114 from zivid/MISC-2024-08-22-fix-crash-on-…

…humble

Fix use of uninitialized memory in zivid_camera

zivid_camera/include/zivid_camera/zivid_camera.hpp

@@ -145,7 +145,6 @@ class ZividCamera : public rclcpp::Node
     const Zivid::CameraIntrinsics & intrinsics);
   [[noreturn]] void logErrorAndThrowRuntimeException(const std::string & message);
 
-  OnSetParametersCallbackHandle::SharedPtr set_parameters_callback_handle_;
   rclcpp::TimerBase::SharedPtr camera_connection_keepalive_timer_;
   bool use_latched_publisher_for_points_xyz_{false};
   bool use_latched_publisher_for_points_xyzrgba_{false};
@@ -174,6 +173,10 @@ class ZividCamera : public rclcpp::Node
   CameraStatus camera_status_{CameraStatus::Idle};
   std::unique_ptr<CaptureSettingsController<Zivid::Settings>> settings_controller_;
   std::unique_ptr<CaptureSettingsController<Zivid::Settings2D>> settings_2d_controller_;
+  // The callback must be declared after the settings controllers since the callback references
+  // both controllers. Otherwise, the callback could run before the controllers are initialized,
+  // which is undefined behavior.
+  OnSetParametersCallbackHandle::SharedPtr set_parameters_callback_handle_;
   std::unique_ptr<Zivid::Camera> camera_;
   std::string frame_id_;
 };

zivid_camera/src/zivid_camera.cpp

@@ -299,12 +299,9 @@ class CaptureSettingsController
 
 ZividCamera::ZividCamera(const rclcpp::NodeOptions & options)
 : rclcpp::Node{"zivid_camera", options},
-  set_parameters_callback_handle_{this->add_on_set_parameters_callback(
-    std::bind(&ZividCamera::setParametersCallback, this, std::placeholders::_1))},
   zivid_{std::make_unique<Zivid::Application>(makeZividApplication())},
-  camera_status_{CameraStatus::Idle},
-  settings_controller_{std::make_unique<CaptureSettingsController<Zivid::Settings>>(*this)},
-  settings_2d_controller_{std::make_unique<CaptureSettingsController<Zivid::Settings2D>>(*this)}
+  set_parameters_callback_handle_{this->add_on_set_parameters_callback(
+    std::bind(&ZividCamera::setParametersCallback, this, std::placeholders::_1))}
 {
   // Disable buffering on stdout
   setvbuf(stdout, nullptr, _IONBF, BUFSIZ);
@@ -313,6 +310,14 @@ ZividCamera::ZividCamera(const rclcpp::NodeOptions & options)
   RCLCPP_INFO(get_logger(), "The node's namespace is '%s'", get_namespace());
   RCLCPP_INFO_STREAM(get_logger(), "Running Zivid Core version " << ZIVID_CORE_VERSION);
 
+  // The "set parameters" callback references both controllers so the pointers must be initialized
+  // before the callback is added. This is because constructing the controllers will cause the "set
+  // parameters" event to fire, and we want to handle that first event in order to log it.
+  // Therefore, we ensure the controllers are initialized to nullptr before initializing the
+  // callback in the initializer list and then construct the controllers here afterward.
+  settings_controller_ = std::make_unique<CaptureSettingsController<Zivid::Settings>>(*this);
+  settings_2d_controller_ = std::make_unique<CaptureSettingsController<Zivid::Settings2D>>(*this);
+
   const auto serial_number = declare_parameter<std::string>(ParamNames::serial_number, "");
 
   frame_id_ = declare_parameter<std::string>(ParamNames::frame_id, "zivid_optical_frame");