Lack of Null Pointer Checks in MessageCacheCircularBuffer::push Causes Crash

[zhihaoshang]

Description

The MessageCacheCircularBuffer::push function does not validate whether the input message pointer is null

Expected Behavior

no crash

Actual Behavior

crash

To Reproduce

Test Case

#include <gtest/gtest.h>
#include <memory>
#include "rosbag2_cpp/cache/message_cache_circular_buffer.hpp"
using namespace rosbag2_cpp::cache;
class MessageCacheCircularBufferTest : public testing::Test {
protected:
  MessageCacheCircularBufferTest() : buffer_(1024) {}  
  MessageCacheCircularBuffer buffer_;
};

TEST_F(MessageCacheCircularBufferTest, PushNullptrCausesCrash) {
  ASSERT_THROW({
    buffer_.push(nullptr);
  }, std::exception); 
}

Output

==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from MessageCacheCircularBufferTest
[ RUN      ] MessageCacheCircularBufferTest.PushNullptrCausesCrash
AddressSanitizer:DEADLYSIGNAL
=================================================================
==50347==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x77fb2f79c05c bp 0x7ffe5a9352f0 sp 0x7ffe5a9352f0 T0)
==50347==The signal is caused by a READ memory access.
==50347==Hint: address points to the zero page.
    #0 0x77fb2f79c05c in std::__shared_ptr<rcutils_uint8_array_s, (__gnu_cxx::_Lock_policy)2>::get() const (/home/shangzh/ros2_jazzy/install/rosbag2_cpp/lib/librosbag2_cpp.so+0x19c05c) (BuildId: 3d08948fe755a0c3db5601d87d0d7a6d5d33654f)
    #1 0x77fb2f79bd01 in std::__shared_ptr_access<rcutils_uint8_array_s, (__gnu_cxx::_Lock_policy)2, false, false>::_M_get() const (/home/shangzh/ros2_jazzy/install/rosbag2_cpp/lib/librosbag2_cpp.so+0x19bd01) (BuildId: 3d08948fe755a0c3db5601d87d0d7a6d5d33654f)
    #2 0x77fb2f79bafb in std::__shared_ptr_access<rcutils_uint8_array_s, (__gnu_cxx::_Lock_policy)2, false, false>::operator->() const (/home/shangzh/ros2_jazzy/install/rosbag2_cpp/lib/librosbag2_cpp.so+0x19bafb) (BuildId: 3d08948fe755a0c3db5601d87d0d7a6d5d33654f)
    #3 0x77fb2f79c999 in rosbag2_cpp::cache::MessageCacheCircularBuffer::push(std::shared_ptr<rosbag2_storage::SerializedBagMessage const>) (/home/shangzh/ros2_jazzy/install/rosbag2_cpp/lib/librosbag2_cpp.so+0x19c999) (BuildId: 3d08948fe755a0c3db5601d87d0d7a6d5d33654f)
    #4 0x5e16f41c54a7 in MessageCacheCircularBufferTest_PushNullptrCausesCrash_Test::TestBody() (/home/shangzh/rosbag2_ws/build/rosbag2_cpp/test_circular_message_cache+0xad4a7) (BuildId: 54ffc90f21d2cdccd50220554d13d915b9679ba9)
    #5 0x5e16f42663df in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (/home/shangzh/rosbag2_ws/build/rosbag2_cpp/test_circular_message_cache+0x14e3df) (BuildId: 54ffc90f21d2cdccd50220554d13d915b9679ba9)
    #6 0x5e16f42534eb in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (/home/shangzh/rosbag2_ws/build/rosbag2_cpp/test_circular_message_cache+0x13b4eb) (BuildId: 54ffc90f21d2cdccd50220554d13d915b9679ba9)
    #7 0x5e16f41f8be9 in testing::Test::Run() (/home/shangzh/rosbag2_ws/build/rosbag2_cpp/test_circular_message_cache+0xe0be9) (BuildId: 54ffc90f21d2cdccd50220554d13d915b9679ba9)
    #8 0x5e16f41fa3c5 in testing::TestInfo::Run() (/home/shangzh/rosbag2_ws/build/rosbag2_cpp/test_circular_message_cache+0xe23c5) (BuildId: 54ffc90f21d2cdccd50220554d13d915b9679ba9)
    #9 0x5e16f41fb71e in testing::TestSuite::Run() (/home/shangzh/rosbag2_ws/build/rosbag2_cpp/test_circular_message_cache+0xe371e) (BuildId: 54ffc90f21d2cdccd50220554d13d915b9679ba9)
    #10 0x5e16f4222449 in testing::internal::UnitTestImpl::RunAllTests() (/home/shangzh/rosbag2_ws/build/rosbag2_cpp/test_circular_message_cache+0x10a449) (BuildId: 54ffc90f21d2cdccd50220554d13d915b9679ba9)
    #11 0x5e16f426983a in bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) (/home/shangzh/rosbag2_ws/build/rosbag2_cpp/test_circular_message_cache+0x15183a) (BuildId: 54ffc90f21d2cdccd50220554d13d915b9679ba9)
    #12 0x5e16f4256788 in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) (/home/shangzh/rosbag2_ws/build/rosbag2_cpp/test_circular_message_cache+0x13e788) (BuildId: 54ffc90f21d2cdccd50220554d13d915b9679ba9)
    #13 0x5e16f421ea53 in testing::UnitTest::Run() (/home/shangzh/rosbag2_ws/build/rosbag2_cpp/test_circular_message_cache+0x106a53) (BuildId: 54ffc90f21d2cdccd50220554d13d915b9679ba9)
    #14 0x5e16f41c8ef4 in RUN_ALL_TESTS() (/home/shangzh/rosbag2_ws/build/rosbag2_cpp/test_circular_message_cache+0xb0ef4) (BuildId: 54ffc90f21d2cdccd50220554d13d915b9679ba9)
    #15 0x5e16f41c8e40 in main (/home/shangzh/rosbag2_ws/build/rosbag2_cpp/test_circular_message_cache+0xb0e40) (BuildId: 54ffc90f21d2cdccd50220554d13d915b9679ba9)
    #16 0x77fb2d62a1c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    #17 0x77fb2d62a28a in __libc_start_main_impl ../csu/libc-start.c:360
    #18 0x5e16f41c5234 in _start (/home/shangzh/rosbag2_ws/build/rosbag2_cpp/test_circular_message_cache+0xad234) (BuildId: 54ffc90f21d2cdccd50220554d13d915b9679ba9)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV (/home/shangzh/ros2_jazzy/install/rosbag2_cpp/lib/librosbag2_cpp.so+0x19c05c) (BuildId: 3d08948fe755a0c3db5601d87d0d7a6d5d33654f) in std::__shared_ptr<rcutils_uint8_array_s, (__gnu_cxx::_Lock_policy)2>::get() const
==50347==ABORTING

System (please complete the following information)

OS: ubuntu 24.04
ROS 2 Distro: ros 2 jazzy
Install Method: source
Version: ros 2 jazzy
build options: --mixin asan-gcc

[MichaelOrlov]

• The push method definded as bool MessageCacheCircularBuffer::push(CacheBufferInterface::buffer_element_t msg), where the buffer_element_t defined as using buffer_element_t = std::shared_ptr<const rosbag2_storage::SerializedBagMessage>; .
  And we are calling push(..) method only from one place
  

  rosbag2/rosbag2_cpp/src/rosbag2_cpp/cache/circular_message_cache.cpp

  Lines 43 to 50 in 951d175

  	bool CircularMessageCache::push(std::shared_ptr<const rosbag2_storage::SerializedBagMessage> msg)
  	{
  	std::lock_guard<std::mutex> cache_lock(producer_buffer_mutex_);
  	producer_buffer_->push(msg);
  	// Always return true since circular message cache drops old messages by design and it
  	// shouldn't be counted as a lost messages.
  	return true;
  	}

• We can add a check for a nullptr as defensive programming. However, it should be guaranteed by the upper layers by design that the shared pointer to the serialized message is not nullptr.

[MichaelOrlov]

• Closing as it wouldn't be fixed. Because the shared pointer is guaranteed to be not null by design from the upper layers.

In consideration of the cases when we would fail to allocate memory for the shared pointer on the upper layer, that is, it would be a situation where something is inherently wrong and a better way to catch a segfault with access by nullptr rather than hiding this issue.

[MichaelOrlov]

• Big kudos for @jncfa for addressing this issue with the possible nullptr access in the Check for nullptrs when pushing new messages to the message cache #2219