From d31e36803f7474bfd773e5dc6ad8eee1bb7d5cc7 Mon Sep 17 00:00:00 2001 From: Jan Kolarik Date: Fri, 24 Nov 2023 14:06:16 +0100 Subject: [PATCH 1/2] Check serialized temporary files memory is non-empty Based on the user report, adding a check after the toml serialization of temporary files memory to make sure the result is not an empty file. Such a file would be not parsable when loading from the disk afterwards. --- libdnf5/base/transaction.cpp | 2 +- libdnf5/repo/package_downloader.cpp | 2 +- libdnf5/repo/temp_files_memory.cpp | 19 +++++++++-- libdnf5/repo/temp_files_memory.hpp | 6 +++- test/libdnf5/repo/test_package_downloader.cpp | 2 +- test/libdnf5/repo/test_temp_files_memory.cpp | 32 ++++++++----------- test/libdnf5/repo/test_temp_files_memory.hpp | 7 ++-- 7 files changed, 39 insertions(+), 31 deletions(-) diff --git a/libdnf5/base/transaction.cpp b/libdnf5/base/transaction.cpp index 5fdaeeac8..479052662 100644 --- a/libdnf5/base/transaction.cpp +++ b/libdnf5/base/transaction.cpp @@ -882,7 +882,7 @@ Transaction::TransactionRunResult Transaction::Impl::_run( auto keepcache = config.get_keepcache_option().get_value(); auto any_inbound_action_present = contains_any_inbound_package(packages); if (!keepcache && any_inbound_action_present) { - libdnf5::repo::TempFilesMemory temp_files_memory(config.get_cachedir_option().get_value()); + libdnf5::repo::TempFilesMemory temp_files_memory(base, config.get_cachedir_option().get_value()); auto temp_files = temp_files_memory.get_files(); for (auto & file : temp_files) { try { diff --git a/libdnf5/repo/package_downloader.cpp b/libdnf5/repo/package_downloader.cpp index 3495221a4..bd61e5a1b 100644 --- a/libdnf5/repo/package_downloader.cpp +++ b/libdnf5/repo/package_downloader.cpp @@ -209,7 +209,7 @@ void PackageDownloader::download() try { }); auto & cachedir = config.get_cachedir_option().get_value(); - TempFilesMemory temp_files_memory(cachedir); + TempFilesMemory temp_files_memory(p_impl->base, cachedir); temp_files_memory.add_files(package_paths); } diff --git a/libdnf5/repo/temp_files_memory.cpp b/libdnf5/repo/temp_files_memory.cpp index f48babf11..4b2d69697 100644 --- a/libdnf5/repo/temp_files_memory.cpp +++ b/libdnf5/repo/temp_files_memory.cpp @@ -20,7 +20,9 @@ along with libdnf. If not, see . #include "temp_files_memory.hpp" #include "utils/fs/file.hpp" +#include "utils/string.hpp" +#include "libdnf5/base/base.hpp" #include "libdnf5/common/exception.hpp" #include "libdnf5/utils/bgettext/bgettext-mark-domain.h" @@ -29,8 +31,9 @@ along with libdnf. If not, see . namespace libdnf5::repo { -TempFilesMemory::TempFilesMemory(const std::string & parent_dir) - : full_memory_path(std::filesystem::path(parent_dir) / MEMORY_FILENAME) { +TempFilesMemory::TempFilesMemory(const BaseWeakPtr & base, const std::string & parent_dir) + : base(base), + full_memory_path(std::filesystem::path(parent_dir) / MEMORY_FILENAME) { std::filesystem::create_directories(parent_dir); } @@ -62,7 +65,17 @@ void TempFilesMemory::add_files(const std::vector & paths) { // write new contents to a temporary file and then move the new file atomically auto temporary_path = full_memory_path.string() + ".temp"; - utils::fs::File(temporary_path, "w").write(toml::format(toml::value({{FILE_PATHS_TOML_KEY, files}}))); + auto new_data = toml::format(toml::value({{FILE_PATHS_TOML_KEY, files}})); + // Although it's not clear for the documentation if it is possible, + // it occurred, that the file was empty, which results in parsing error, + // see https://github.com/rpm-software-management/dnf5/issues/1001 + if (new_data.empty()) { + auto & logger = *base->get_logger(); + logger.error( + "Serializing temporary file paths failed for the input vector: \"{}\"", utils::string::join(files, ", ")); + return; + } + utils::fs::File(temporary_path, "w").write(new_data); std::filesystem::rename(temporary_path, full_memory_path); } diff --git a/libdnf5/repo/temp_files_memory.hpp b/libdnf5/repo/temp_files_memory.hpp index 79179097b..2b18811a6 100644 --- a/libdnf5/repo/temp_files_memory.hpp +++ b/libdnf5/repo/temp_files_memory.hpp @@ -20,6 +20,8 @@ along with libdnf. If not, see . #ifndef LIBDNF5_REPO_TEMP_FILES_MEMORY_HPP #define LIBDNF5_REPO_TEMP_FILES_MEMORY_HPP +#include "libdnf5/base/base_weak.hpp" + #include #include @@ -38,10 +40,11 @@ class TempFilesMemory { static constexpr const char * FILE_PATHS_TOML_KEY = "files"; /// @brief Create the object for managing temporary files' paths. + /// @param base A weak pointer to Base object. /// @param parent_dir Path to a directory where the memory file is or will be stored. /// If the directory doesn't exist yet, it will be created. /// @exception std::filesystem::filesystem_error When an error occurs during creating the parent directory. - TempFilesMemory(const std::string & parent_dir); + TempFilesMemory(const BaseWeakPtr & base, const std::string & parent_dir); ~TempFilesMemory(); /// @brief Retrieve stored paths of temporary files. @@ -64,6 +67,7 @@ class TempFilesMemory { void clear(); private: + BaseWeakPtr base; std::filesystem::path full_memory_path; }; diff --git a/test/libdnf5/repo/test_package_downloader.cpp b/test/libdnf5/repo/test_package_downloader.cpp index 7fe7bd0d7..b7593750e 100644 --- a/test/libdnf5/repo/test_package_downloader.cpp +++ b/test/libdnf5/repo/test_package_downloader.cpp @@ -107,7 +107,7 @@ void PackageDownloaderTest::test_package_downloader_temp_files_memory() { config.get_keepcache_option().set(false); auto & cachedir = config.get_cachedir_option().get_value(); - libdnf5::repo::TempFilesMemory memory(cachedir); + libdnf5::repo::TempFilesMemory memory(base.get_weak_ptr(), cachedir); // check memory is empty before downloading CPPUNIT_ASSERT_EQUAL((size_t)0, memory.get_files().size()); diff --git a/test/libdnf5/repo/test_temp_files_memory.cpp b/test/libdnf5/repo/test_temp_files_memory.cpp index 254da680b..c1437c277 100644 --- a/test/libdnf5/repo/test_temp_files_memory.cpp +++ b/test/libdnf5/repo/test_temp_files_memory.cpp @@ -32,36 +32,30 @@ using namespace libdnf5::repo; void TempFilesMemoryTest::setUp() { - CppUnit::TestCase::setUp(); - temp_dir = std::make_unique("libdnf_test_filesmemory"); - parent_dir_path = temp_dir->get_path(); + BaseTestCase::setUp(); + parent_dir_path = temp->get_path(); full_path = parent_dir_path / TempFilesMemory::MEMORY_FILENAME; } -void TempFilesMemoryTest::tearDown() { - temp_dir.reset(); - CppUnit::TestCase::tearDown(); -} - void TempFilesMemoryTest::test_directory_is_created_when_not_exists() { - auto test_path = temp_dir->get_path() / "some/new/path"; + auto test_path = parent_dir_path / "some/new/path"; CPPUNIT_ASSERT(!std::filesystem::exists(test_path)); - TempFilesMemory memory(test_path); + TempFilesMemory memory(base.get_weak_ptr(), test_path); CPPUNIT_ASSERT(std::filesystem::exists(test_path)); } void TempFilesMemoryTest::test_get_files_when_empty_storage() { - TempFilesMemory memory(temp_dir->get_path() / "unknown/path"); + TempFilesMemory memory(base.get_weak_ptr(), parent_dir_path / "unknown/path"); CPPUNIT_ASSERT(memory.get_files().empty()); } void TempFilesMemoryTest::test_get_files_throws_exception_when_invalid_format() { libdnf5::utils::fs::File(full_path, "w").write(""); - TempFilesMemory memory_empty(parent_dir_path); + TempFilesMemory memory_empty(base.get_weak_ptr(), parent_dir_path); CPPUNIT_ASSERT_THROW(memory_empty.get_files(), libdnf5::Error); libdnf5::utils::fs::File(full_path, "w").write("[\"path1\", \"path2\", \"path3\"]"); - TempFilesMemory memory_invalid(parent_dir_path); + TempFilesMemory memory_invalid(base.get_weak_ptr(), parent_dir_path); CPPUNIT_ASSERT_THROW(memory_invalid.get_files(), libdnf5::Error); } @@ -70,7 +64,7 @@ void TempFilesMemoryTest::test_get_files_returns_stored_values() { .write(fmt::format( "{} = [\"path/to/package1.rpm\", \"different/path/to/package2.rpm\"]", TempFilesMemory::FILE_PATHS_TOML_KEY)); - TempFilesMemory memory(parent_dir_path); + TempFilesMemory memory(base.get_weak_ptr(), parent_dir_path); std::vector expected = {"path/to/package1.rpm", "different/path/to/package2.rpm"}; CPPUNIT_ASSERT_EQUAL(expected, memory.get_files()); } @@ -78,7 +72,7 @@ void TempFilesMemoryTest::test_get_files_returns_stored_values() { void TempFilesMemoryTest::test_add_files_when_empty_storage() { std::vector new_paths = {"path1", "path2"}; - TempFilesMemory memory(parent_dir_path); + TempFilesMemory memory(base.get_weak_ptr(), parent_dir_path); CPPUNIT_ASSERT(memory.get_files().empty()); memory.add_files(new_paths); @@ -90,7 +84,7 @@ void TempFilesMemoryTest::test_add_files_when_existing_storage() { libdnf5::utils::fs::File(full_path, "w") .write(fmt::format("{} = [\"path1\", \"path2\"]", TempFilesMemory::FILE_PATHS_TOML_KEY)); - TempFilesMemory memory(parent_dir_path); + TempFilesMemory memory(base.get_weak_ptr(), parent_dir_path); memory.add_files(new_paths); std::vector expected = {"path1", "path2", "path3", "path4"}; CPPUNIT_ASSERT_EQUAL(expected, memory.get_files()); @@ -101,7 +95,7 @@ void TempFilesMemoryTest::test_add_files_deduplicates_and_sorts_data() { libdnf5::utils::fs::File(full_path, "w") .write(fmt::format("{} = [\"path4\", \"path1\", \"path4\", \"path3\"]", TempFilesMemory::FILE_PATHS_TOML_KEY)); - TempFilesMemory memory(parent_dir_path); + TempFilesMemory memory(base.get_weak_ptr(), parent_dir_path); memory.add_files(new_paths); std::vector expected = {"path1", "path2", "path3", "path4"}; CPPUNIT_ASSERT_EQUAL(expected, memory.get_files()); @@ -113,7 +107,7 @@ void TempFilesMemoryTest::test_clear_deletes_storage_content() { "{} = [\"/path/to/package1.rpm\", \"/different-path/to/package2.rpm\", " "\"/another-path/leading/to/pkg3.rpm\"]", TempFilesMemory::FILE_PATHS_TOML_KEY)); - TempFilesMemory memory(parent_dir_path); + TempFilesMemory memory(base.get_weak_ptr(), parent_dir_path); std::vector expected = { "/path/to/package1.rpm", "/different-path/to/package2.rpm", "/another-path/leading/to/pkg3.rpm"}; CPPUNIT_ASSERT_EQUAL(expected, memory.get_files()); @@ -123,6 +117,6 @@ void TempFilesMemoryTest::test_clear_deletes_storage_content() { } void TempFilesMemoryTest::test_clear_when_empty_storage() { - TempFilesMemory memory(temp_dir->get_path() / "non-existing/path"); + TempFilesMemory memory(base.get_weak_ptr(), parent_dir_path / "non-existing/path"); memory.clear(); } diff --git a/test/libdnf5/repo/test_temp_files_memory.hpp b/test/libdnf5/repo/test_temp_files_memory.hpp index 470a7035e..42f23f983 100644 --- a/test/libdnf5/repo/test_temp_files_memory.hpp +++ b/test/libdnf5/repo/test_temp_files_memory.hpp @@ -20,15 +20,14 @@ along with libdnf. If not, see . #ifndef LIBDNF5_TEST_REPO_TEMP_FILES_MEMORY_HPP #define LIBDNF5_TEST_REPO_TEMP_FILES_MEMORY_HPP -#include "utils/fs/temp.hpp" +#include "../shared/base_test_case.hpp" -#include #include #include -class TempFilesMemoryTest : public CppUnit::TestCase { +class TempFilesMemoryTest : public BaseTestCase { CPPUNIT_TEST_SUITE(TempFilesMemoryTest); CPPUNIT_TEST(test_directory_is_created_when_not_exists); CPPUNIT_TEST(test_get_files_when_empty_storage); @@ -43,7 +42,6 @@ class TempFilesMemoryTest : public CppUnit::TestCase { public: void setUp() override; - void tearDown() override; void test_directory_is_created_when_not_exists(); void test_get_files_when_empty_storage(); @@ -56,7 +54,6 @@ class TempFilesMemoryTest : public CppUnit::TestCase { void test_clear_when_empty_storage(); private: - std::unique_ptr temp_dir; std::filesystem::path parent_dir_path; std::filesystem::path full_path; }; From de29b505c006f21c481894411041ddb1e90ea4de Mon Sep 17 00:00:00 2001 From: Jan Kolarik Date: Fri, 24 Nov 2023 14:08:13 +0100 Subject: [PATCH 2/2] Test for adding an empty list to memory file Although this scenario is not expected, make sure it is not crashing. --- test/libdnf5/repo/test_temp_files_memory.cpp | 11 +++++++++++ test/libdnf5/repo/test_temp_files_memory.hpp | 2 ++ 2 files changed, 13 insertions(+) diff --git a/test/libdnf5/repo/test_temp_files_memory.cpp b/test/libdnf5/repo/test_temp_files_memory.cpp index c1437c277..428c54221 100644 --- a/test/libdnf5/repo/test_temp_files_memory.cpp +++ b/test/libdnf5/repo/test_temp_files_memory.cpp @@ -79,6 +79,17 @@ void TempFilesMemoryTest::test_add_files_when_empty_storage() { CPPUNIT_ASSERT_EQUAL(new_paths, memory.get_files()); } +void TempFilesMemoryTest::test_add_no_files_when_empty_storage() { + // make sure that adding an empty vector will not cause crashing + + TempFilesMemory memory(base.get_weak_ptr(), parent_dir_path); + CPPUNIT_ASSERT(memory.get_files().empty()); + + std::vector empty_paths = {}; + memory.add_files(empty_paths); + CPPUNIT_ASSERT(memory.get_files().empty()); +} + void TempFilesMemoryTest::test_add_files_when_existing_storage() { std::vector new_paths = {"path3", "path4"}; libdnf5::utils::fs::File(full_path, "w") diff --git a/test/libdnf5/repo/test_temp_files_memory.hpp b/test/libdnf5/repo/test_temp_files_memory.hpp index 42f23f983..f552f257f 100644 --- a/test/libdnf5/repo/test_temp_files_memory.hpp +++ b/test/libdnf5/repo/test_temp_files_memory.hpp @@ -34,6 +34,7 @@ class TempFilesMemoryTest : public BaseTestCase { CPPUNIT_TEST(test_get_files_throws_exception_when_invalid_format); CPPUNIT_TEST(test_get_files_returns_stored_values); CPPUNIT_TEST(test_add_files_when_empty_storage); + CPPUNIT_TEST(test_add_no_files_when_empty_storage); CPPUNIT_TEST(test_add_files_when_existing_storage); CPPUNIT_TEST(test_add_files_deduplicates_and_sorts_data); CPPUNIT_TEST(test_clear_deletes_storage_content); @@ -48,6 +49,7 @@ class TempFilesMemoryTest : public BaseTestCase { void test_get_files_throws_exception_when_invalid_format(); void test_get_files_returns_stored_values(); void test_add_files_when_empty_storage(); + void test_add_no_files_when_empty_storage(); void test_add_files_when_existing_storage(); void test_add_files_deduplicates_and_sorts_data(); void test_clear_deletes_storage_content();