fix: add nullptr check when pushing new messages to the message cache

Signed-off-by: Jose Faria <[email protected]>

Author: jncfa

rosbag2_cpp/src/rosbag2_cpp/cache/circular_message_cache.cpp

@@ -42,6 +42,10 @@ CircularMessageCache::~CircularMessageCache()
 
 bool CircularMessageCache::push(std::shared_ptr<const rosbag2_storage::SerializedBagMessage> msg)
 {
+  if (msg == nullptr) {
+    return false;
+  }
+
   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

rosbag2_cpp/src/rosbag2_cpp/cache/message_cache_circular_buffer.cpp

@@ -32,6 +32,10 @@ MessageCacheCircularBuffer::MessageCacheCircularBuffer(size_t max_cache_size)
 
 bool MessageCacheCircularBuffer::push(CacheBufferInterface::buffer_element_t msg)
 {
+  if (msg == nullptr) {
+    return false;
+  }
+
   // Drop message if it exceeds the buffer size
   if (msg->serialized_data->buffer_length > max_bytes_size_) {
     ROSBAG2_CPP_LOG_WARN_STREAM("Last message exceeds snapshot buffer size. Dropping message!");

rosbag2_cpp/test/rosbag2_cpp/test_message_cache.cpp

@@ -13,7 +13,7 @@
 // limitations under the License.
 
 #include <gmock/gmock.h>
-
+#include <gtest/gtest.h>
 #include <chrono>
 #include <numeric>
 #include <memory>
@@ -23,12 +23,14 @@
 #include <vector>
 #include <thread>
 
+#include "rosbag2_cpp/cache/message_cache_circular_buffer.hpp"
 #include "rosbag2_storage/ros_helper.hpp"
 #include "rosbag2_storage/serialized_bag_message.hpp"
 
 #include "mock_cache_consumer.hpp"
 #include "mock_message_cache.hpp"
 
+
 using namespace testing;  // NOLINT
 
 namespace
@@ -105,3 +107,18 @@ TEST_F(MessageCacheTest, message_cache_writes_full_producer_buffer) {
   mock_cache_consumer->stop();
   EXPECT_EQ(consumed_message_count, message_count - should_be_dropped_count);
 }
+
+class MessageCacheCircularBufferTest : public testing::Test {
+protected:
+  MessageCacheCircularBufferTest()
+  : buffer_(1024) {}
+  rosbag2_cpp::cache::MessageCacheCircularBuffer buffer_;
+};
+
+TEST_F(MessageCacheCircularBufferTest, PushNullptrCausesCrash) {
+  bool result = true;
+  ASSERT_NO_THROW({
+    result = buffer_.push(nullptr);
+  });
+  EXPECT_FALSE(result);
+}

rosbag2_storage/CMakeLists.txt

@@ -133,6 +133,13 @@ if(BUILD_TESTING)
     rosbag2_test_common::rosbag2_test_common
     yaml-cpp::yaml-cpp
   )
+  
+  ament_add_gmock(test_cache
+    test/rosbag2_storage/test_cache.cpp)
+  target_link_libraries(test_cache
+    ${PROJECT_NAME}
+    rosbag2_test_common::rosbag2_test_common
+  )
 endif()
 
 ament_package()