Bugfix/segfault when getting surrounding interval of empty cache #116
Conversation
include/message_filters/cache.h
Outdated
There was a problem hiding this comment.
segfault occurs in line 229/235, when cache size is empty.
start_index and end_index are both -1, the current logic will then reserve -1 - (-1) + 1 = 1 element and try to push_back(cache_[-1]...);.
test/msg_cache_unittest.cpp
Outdated
| TEST(Cache, emptySurroundingInterval) | ||
| { | ||
| message_filters::Cache<Msg> cache(10); | ||
| const std::vector<std::shared_ptr<Msg const> > interval_data = cache.getSurroundingInterval(rclcpp::Time(5, 0), rclcpp::Time(9, 0)); |
There was a problem hiding this comment.
line too long, it should be less than 100 characters
Cakem1x
force-pushed
the
bugfix/segfault-when-getting-surrounding-interval-of-empty-cache
branch
from
1c9b0f5
to
d89350d
Compare
fixes segfault in Cache::getSurroundingInterval
Cakem1x
force-pushed
the
bugfix/segfault-when-getting-surrounding-interval-of-empty-cache
branch
from
d89350d
to
dd5bb14
Compare
There was a problem hiding this comment.
Still some linters failures https://build.ros2.org/job/Rpr__message_filters__ubuntu_noble_amd64/87/#showFailuresLink
Right, sorry, I had some issues with my local |
|
Just out of curiosity, do you use some specific tool (via IDE or CLI) to automatically format code to your CI's liking? |
test/msg_cache_unittest.cpp
Outdated
| rclcpp::Time( | ||
| 5, | ||
| 0), rclcpp::Time( | ||
| 9, 0)); |
There was a problem hiding this comment.
Indentation looks odd. Did you try something like this ?
| rclcpp::Time( | |
| 5, | |
| 0), rclcpp::Time( | |
| 9, 0)); | |
| rclcpp::Time(5, 0), rclcpp::Time(9, 0)); |
test/msg_cache_unittest.cpp
Outdated
| 0), rclcpp::Time( | ||
| 9, 0)); | ||
|
|
||
| EXPECT_EQ(interval_data.size(), (unsigned int) 0); // empty cache shall return empty interval |
There was a problem hiding this comment.
| EXPECT_EQ(interval_data.size(), (unsigned int) 0); // empty cache shall return empty interval | |
| EXPECT_EQ(interval_data.size(), static_cast<unsigned int>(0)); // empty cache shall return empty interval |
|
Looks like something is up with Jenkins, could you please re-trigger the job (when Jenkins is feeling better)? I think the failure is unrelated to the changes in this PR. |
|
|
@ros-pull-request-builder retest this please |
|
@Cakem1x Thank you for iterating and the contribution |
|
https://github.com/Mergifyio backport jazzy |
✅ Backports have been created
|
(cherry picked from commit e60450d) # Conflicts: # include/message_filters/cache.hpp
This PR fixes a segfault that occurred to me when I tried to get a surrounding interval from a
message_filter::Cachethat was empty. See comment below for where/how.The PR includes
Is it possible to get the fix also to other branches, specifically
humble(butironwould probably also make sense)? That's the version I am using at the moment. I think the fix commit should be cherry-pickable. But the test commit probably needs some minor adaptation due to namespace changes.Please let me know if I need to follow some specific procedure or provide more information for contributing (e.g. create issue first, or similar).