Sanitize bagfile splitting CLI input (ros2#226)

* Sanitize bagfile splitting CLI input Signed-off-by: Prajakta Gokhale <[email protected]> * Fix test_constants default value Signed-off-by: Prajakta Gokhale <[email protected]>
agutenkunst · Jan 3, 2020 · cbb9fd7 · cbb9fd7
1 parent 226971e
commit cbb9fd7
Show file tree

Hide file tree

Showing 9 changed files with 47 additions and 0 deletions.
diff --git a/rosbag2/src/rosbag2/writers/sequential_writer.cpp b/rosbag2/src/rosbag2/writers/sequential_writer.cpp
@@ -93,6 +93,13 @@ void SequentialWriter::open(
     throw std::runtime_error("No storage could be initialized. Abort");
   }
 
+  if (max_bagfile_size_ != 0 &&
+    max_bagfile_size_ < storage_->get_minimum_split_file_size())
+  {
+    throw std::runtime_error(
+            "Invalid bag splitting size given. Please provide a different value.");
+  }
+
   init_metadata();
 }
 

diff --git a/rosbag2/test/rosbag2/mock_storage.hpp b/rosbag2/test/rosbag2/mock_storage.hpp
@@ -40,6 +40,7 @@ class MockStorage : public rosbag2_storage::storage_interfaces::ReadWriteInterfa
   MOCK_CONST_METHOD0(get_bagfile_size, uint64_t());
   MOCK_CONST_METHOD0(get_relative_file_path, std::string());
   MOCK_CONST_METHOD0(get_storage_identifier, std::string());
+  MOCK_CONST_METHOD0(get_minimum_split_file_size, uint64_t());
 };
 
 #endif  // ROSBAG2__MOCK_STORAGE_HPP_
diff --git a/rosbag2/test/rosbag2/test_sequential_writer.cpp b/rosbag2/test/rosbag2/test_sequential_writer.cpp
@@ -140,6 +140,24 @@ TEST_F(SequentialWriterTest, open_throws_error_if_converter_plugin_does_not_exis
   EXPECT_ANY_THROW(writer_->open(storage_options_, {input_format, output_format}));
 }
 
+TEST_F(SequentialWriterTest, open_throws_error_on_invalid_splitting_size) {
+  auto sequential_writer = std::make_unique<rosbag2::writers::SequentialWriter>(
+    std::move(storage_factory_), converter_factory_, std::move(metadata_io_));
+  writer_ = std::make_unique<rosbag2::Writer>(std::move(sequential_writer));
+
+  // Set minimum file size greater than max bagfile size option
+  const uint64_t min_split_file_size = 10;
+  const uint64_t max_bagfile_size = 5;
+  ON_CALL(*storage_, get_minimum_split_file_size()).WillByDefault(Return(min_split_file_size));
+  storage_options_.max_bagfile_size = max_bagfile_size;
+
+  EXPECT_CALL(*storage_, get_minimum_split_file_size).Times(1);
+
+  std::string rmw_format = "rmw_format";
+
+  EXPECT_ANY_THROW(writer_->open(storage_options_, {rmw_format, rmw_format}));
+}
+
 TEST_F(SequentialWriterTest, bagfile_size_is_checked_on_every_write) {
   const int counter = 10;
   const uint64_t max_bagfile_size = 100;

diff --git a/rosbag2_storage/include/rosbag2_storage/storage_interfaces/read_write_interface.hpp b/rosbag2_storage/include/rosbag2_storage/storage_interfaces/read_write_interface.hpp
@@ -37,6 +37,8 @@ class ROSBAG2_STORAGE_PUBLIC ReadWriteInterface
   uint64_t get_bagfile_size() const override = 0;
 
   std::string get_storage_identifier() const override = 0;
+
+  virtual uint64_t get_minimum_split_file_size() const = 0;
 };
 
 }  // namespace storage_interfaces

diff --git a/rosbag2_storage/test/rosbag2_storage/test_constants.hpp b/rosbag2_storage/test/rosbag2_storage/test_constants.hpp
@@ -22,6 +22,7 @@ constexpr const char * const READ_WRITE_PLUGIN_IDENTIFIER = "ReadWritePlugin";
 constexpr const char * const READ_ONLY_PLUGIN_IDENTIFIER = "ReadOnlyPlugin";
 constexpr const char * const DUMMY_FILEPATH = "/path/to/storage";
 constexpr const uint64_t MAX_BAGFILE_SIZE = 0;
+constexpr const uint64_t MIN_SPLIT_FILE_SIZE = UINT64_MAX;
 }
 
 #endif  // ROSBAG2_STORAGE__TEST_CONSTANTS_HPP_
diff --git a/rosbag2_storage/test/rosbag2_storage/test_plugin.cpp b/rosbag2_storage/test/rosbag2_storage/test_plugin.cpp
@@ -98,4 +98,10 @@ std::string TestPlugin::get_storage_identifier() const
   return test_constants::READ_WRITE_PLUGIN_IDENTIFIER;
 }
 
+uint64_t TestPlugin::get_minimum_split_file_size() const
+{
+  std::cout << "\nreturning minimum split file size\n";
+  return test_constants::MIN_SPLIT_FILE_SIZE;
+}
+
 PLUGINLIB_EXPORT_CLASS(TestPlugin, rosbag2_storage::storage_interfaces::ReadWriteInterface)
diff --git a/rosbag2_storage/test/rosbag2_storage/test_plugin.hpp b/rosbag2_storage/test/rosbag2_storage/test_plugin.hpp
@@ -50,6 +50,8 @@ class TestPlugin : public rosbag2_storage::storage_interfaces::ReadWriteInterfac
   uint64_t get_bagfile_size() const override;
 
   std::string get_storage_identifier() const override;
+
+  uint64_t get_minimum_split_file_size() const override;
 };
 
 #endif  // ROSBAG2_STORAGE__TEST_PLUGIN_HPP_
diff --git a/...storage_default_plugins/include/rosbag2_storage_default_plugins/sqlite/sqlite_storage.hpp b/...storage_default_plugins/include/rosbag2_storage_default_plugins/sqlite/sqlite_storage.hpp
@@ -70,6 +70,8 @@ class ROSBAG2_STORAGE_DEFAULT_PLUGINS_PUBLIC SqliteStorage
 
   std::string get_storage_identifier() const override;
 
+  uint64_t get_minimum_split_file_size() const override;
+
 private:
   void initialize();
   void prepare_for_writing();

diff --git a/...ag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_storage.cpp b/...ag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_storage.cpp
@@ -55,6 +55,9 @@ bool is_read_write(const rosbag2_storage::storage_interfaces::IOFlag io_flag)
 }
 
 constexpr const auto FILE_EXTENSION = ".db3";
+
+// Minimum size of a sqlite3 database file in bytes (84 kiB).
+constexpr const uint64_t MIN_SPLIT_FILE_SIZE = 86016;
 }  // namespace
 
 namespace rosbag2_storage_plugins
@@ -236,6 +239,11 @@ std::string SqliteStorage::get_relative_file_path() const
   return relative_path_;
 }
 
+uint64_t SqliteStorage::get_minimum_split_file_size() const
+{
+  return MIN_SPLIT_FILE_SIZE;
+}
+
 rosbag2_storage::BagMetadata SqliteStorage::get_metadata()
 {
   rosbag2_storage::BagMetadata metadata;