Skip to content
This repository has been archived by the owner on Jul 1, 2024. It is now read-only.

fix(lanelet2_extension): invalid access #240

Merged
merged 1 commit into from
Mar 4, 2024
Merged

fix(lanelet2_extension): invalid access #240

merged 1 commit into from
Mar 4, 2024

Conversation

shmpwk
Copy link
Contributor

@shmpwk shmpwk commented Mar 3, 2024

Description

This PR prevents getLineStringFromArcLength from SIGSEGV.
When linestring.size() is 0, linestring.size() - 1 can be inf becauselinestring.size() is unsigned .

for (size_t i = 0; i < linestring.size() - 1; i++) {

As a result, linestring[i] can be invalid access.

const auto & p1 = linestring[i];

Related links

TIER IV internal slack

Tests performed

We can reproduce this problem.

  1. Download internal rosbag data.
    rosbag0
    rosbag1
    rosbag2
    rosbag3

  2. Prepare environment of pilo-auto.xx1:v0.43.6.0.

  3. Execute ros2 launch autoware_launch logging_simulator.launch.xml map_path:=$HOME/autoware_map/odaiba_beta vehicle_model:=jpntaxi sensor_model:=aip_xx1 localization:=false perception:=false

  4. After launching Step3, play the rosbag from 0 to 3 one by one with remmaping planning topic.
    ./play_bag_without_planning.sh <rosbag>

#!/bin/bash
# play_bag_without_planning.sh

BAG_NAME=$1
PLANNING_TOPIC=$(ros2 bag info $BAG_NAME | awk '{print $2}' | grep planning | grep -v mission_planning)

COMMAND_OPTION='--remap '
for i in ${PLANNING_TOPIC[@]}; do
    TARGET_PLANNING_TOPIC=$i
    RENAMED_TARGET_PLANNING_TOPIC=$(echo $TARGET_PLANNING_TOPIC | sed 's/\/planning/\/tmp\/planning/g')
    COMMAND_OPTION=$COMMAND_OPTION' '$TARGET_PLANNING_TOPIC':='$RENAMED_TARGET_PLANNING_TOPIC
done

ros2 bag play $BAG_NAME $COMMAND_OPTION --clock  -s sqlite3
  1. At the last time, there is SIGSEGV. Also I confirmed linestring.size() - 1 is 18446744073709551615.
[component_container_mt-44] *** Aborted at 1709478740 (unix time) try "date -d @1709478740" if you are using GNU date ***
[component_container_mt-44] PC: @                0x0 (unknown)
[component_container_mt-44] *** SIGSEGV (@0x10) received by PID 315075 (TID 0x7f41daffd640) from PID 16; stack trace: ***
[component_container_mt-44]     @     0x7f41d805d046 (unknown)
[component_container_mt-44]     @     0x7f41f7842520 (unknown)
[component_container_mt-44]     @     0x7f41ec33aa6b lanelet::utils::(anonymous namespace)::getLineStringFromArcLength()
[component_container_mt-44]     @     0x7f41ec33ca73 lanelet::utils::getPolygonFromArcLength()
[component_container_mt-44]     @     0x7f41d9cd61fe behavior_path_planner::NormalLaneChange::getLaneChangePaths()
[component_container_mt-44]     @     0x7f41d9ccd39c behavior_path_planner::NormalLaneChange::getSafePath()
[component_container_mt-44]     @     0x7f41d9cccca9 behavior_path_planner::NormalLaneChange::updateLaneChangeStatus()
[component_container_mt-44]     @     0x7f41d9b23626 behavior_path_planner::PlannerManager::getRequestModules()
[component_container_mt-44]     @     0x7f41d9b29042 _ZZN21behavior_path_planner14PlannerManager3runERKSt10shared_ptrINS_11PlannerDataEEENKUlvE0_clEv
[component_container_mt-44]     @     0x7f41d9b2996c behavior_path_planner::PlannerManager::run()
[component_container_mt-44]     @     0x7f41d9b5d33b behavior_path_planner::BehaviorPathPlannerNode::run()
[component_container_mt-44]     @     0x7f41d9b65965 rclcpp::GenericTimer<>::execute_callback()
[component_container_mt-44]     @     0x7f41f815effe rclcpp::Executor::execute_any_executable()
[component_container_mt-44]     @     0x7f41f8165432 rclcpp::executors::MultiThreadedExecutor::run()
[component_container_mt-44]     @     0x7f41f7cdc253 (unknown)
[component_container_mt-44]     @     0x7f41f7894ac3 (unknown)
[component_container_mt-44]     @     0x7f41f7926850 (unknown)
[component_container_mt-44]     @                0x0 (unknown)
[component_container_mt-27] [ERROR] [1709478740.785274785] [pcl_ros]: Input dataset has no X-Y-Z coordinates! Cannot convert to Eigen format.
[ERROR] [component_container_mt-44]: process has died [pid 315075, exit code -11, cmd '/home/shmpwk/xx1/pilot-auto.xx1.v0.43.6.0/install/rclcpp_components/lib/rclcpp_components/component_container_mt --ros-args -r __node:=behavior_planning_container -r __ns:=/planning/scenario_planning/lane_driving/behavior_planning -p use_sim_time:=True -p wheel_radius:=0.31 -p wheel_width:=0.18 -p wheel_base:=2.75 -p wheel_tread:=1.485 -p front_overhang:=0.8 -p rear_overhang:=0.85 -p left_overhang:=0.105 -p right_overhang:=0.105 -p vehicle_height:=2.5 -p max_steer_angle:=0.55'].

With this PR, I confirmed there is no SIGSEGV.

Notes for reviewers

N/A

Interface changes

N/A

Effects on system behavior

Fix bug

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

  • The PR follows the pull request guidelines.
  • The PR has been properly tested.
  • The PR has been reviewed by the code owners.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • There are no open discussions or they are tracked via tickets.
  • The PR is ready for merge.

After all checkboxes are checked, anyone who has write access can merge the PR.

Comment on lines +216 to +218
if (start_index == 0) {
return lanelet::LineString3d{lanelet::InvalId, points};
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can I ask for more detail about how the node side will behave with this change?
I'm curious about whether the node side will work well with lanelet::Invalid.

Copy link
Contributor Author

@shmpwk shmpwk Mar 3, 2024

Choose a reason for hiding this comment

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

I didn't check how the node behaves actually. But at least this function always returns lanelet::LineString3d{lanelet::InvalId, points}; and I think lanelet::Invalid has no problem.

Copy link

codecov bot commented Mar 3, 2024

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 10.46%. Comparing base (7c55915) to head (0dbf93e).

Files Patch % Lines
tmp/lanelet2_extension/lib/utilities.cpp 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #240      +/-   ##
==========================================
- Coverage   10.46%   10.46%   -0.01%     
==========================================
  Files          42       42              
  Lines        3152     3154       +2     
  Branches     1407     1409       +2     
==========================================
  Hits          330      330              
- Misses       2395     2396       +1     
- Partials      427      428       +1     
Flag Coverage Δ *Carryforward flag
differential 8.13% <0.00%> (?)
total 10.46% <ø> (ø) Carriedforward from 7c55915

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@shmpwk shmpwk marked this pull request as draft March 4, 2024 00:29
@shmpwk shmpwk marked this pull request as ready for review March 4, 2024 01:24
@shmpwk shmpwk requested a review from Ericpotato March 4, 2024 02:36
Copy link
Contributor

@soblin soblin left a comment

Choose a reason for hiding this comment

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

This function should return optional instead of InvalId with empty points, and for more strict handling, we should force the callee side to pass linestring with more than 2 points in the future.

@shmpwk shmpwk merged commit c47c979 into main Mar 4, 2024
24 of 26 checks passed
@shmpwk shmpwk deleted the shmpwk-patch-2 branch March 4, 2024 04:01
shmpwk added a commit that referenced this pull request Mar 4, 2024
shmpwk added a commit to tier4/autoware_common that referenced this pull request Mar 4, 2024
masayukiaino pushed a commit to masayukiaino/autoware_common that referenced this pull request Mar 5, 2024
s-azumi pushed a commit to tier4/autoware_common that referenced this pull request Mar 15, 2024
1222-takeshi pushed a commit to tier4/autoware_common that referenced this pull request Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants