From 872b97f1d305ee37e9815c14a14af7978804f1df Mon Sep 17 00:00:00 2001 From: mutouyun Date: Sun, 22 Oct 2023 17:39:43 +0800 Subject: [PATCH] Add `-fsanitize=address` for memory leak and out-of-bounds detection. --- CMakeLists.txt | 5 ++-- include/libpmr/small_storage.h | 42 ++++++++++++++++---------- src/libipc/platform/posix/shm_impl.h | 16 +++++----- src/libipc/platform/win/mmap_impl.h | 4 +-- src/libipc/shm.cpp | 2 +- src/libpmr/memory_resource.cpp | 4 ++- test/imp/test_imp_byte.cpp | 10 +++++-- test/pmr/test_pmr_small_storage.cpp | 45 ++++++++++++++++++---------- 8 files changed, 79 insertions(+), 49 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 479b6789..d47282fc 100755 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -12,7 +12,7 @@ set(CMAKE_CXX_STANDARD 17) set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} -DNDEBUG") if(NOT MSVC) set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} -O2") - set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_DEBUG} -g -rdynamic") + set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -g -rdynamic -fsanitize=address") endif() if (MSVC) @@ -22,8 +22,7 @@ if (MSVC) CMAKE_CXX_FLAGS_RELEASE CMAKE_C_FLAGS CMAKE_C_FLAGS_DEBUG - CMAKE_C_FLAGS_RELEASE - ) + CMAKE_C_FLAGS_RELEASE) if (LIBIPC_USE_STATIC_CRT) foreach(CompilerFlag ${CompilerFlags}) string(REPLACE "/MD" "/MT" ${CompilerFlag} "${${CompilerFlag}}") diff --git a/include/libpmr/small_storage.h b/include/libpmr/small_storage.h index faff4590..58d6792a 100644 --- a/include/libpmr/small_storage.h +++ b/include/libpmr/small_storage.h @@ -204,6 +204,8 @@ class holder : public holder_base { namespace detail { +/// \struct holder_type_base +/// \brief Defines the holder type operation interface. struct holder_type_base { virtual ~holder_type_base() noexcept = default; virtual std::size_t size() const noexcept = 0; @@ -213,6 +215,8 @@ struct holder_type_base { virtual void dest(void *p, std::size_t n) const noexcept = 0; }; +/// \struct template holder_type +/// \brief Defines generalization operations for different data types. template struct holder_type : holder_type_base { std::size_t size() const noexcept override { return sizeof(Value); } @@ -240,6 +244,10 @@ struct holder_info { std::size_t count; }; +struct holder_data : holder_info { + alignas(std::max_align_t) char ___data; +}; + template std::size_t full_sizeof(std::size_t count) noexcept { return ::LIBIMP::round_up(sizeof(detail::holder_info), alignof(std::max_align_t)) @@ -252,29 +260,31 @@ std::size_t full_sizeof(holder_info const *info) noexcept { } void *value_ptr(holder_info *info) noexcept { - return reinterpret_cast( - ::LIBIMP::round_up(reinterpret_cast(info + 1), alignof(std::max_align_t))); + if (info == nullptr) return nullptr; + return &static_cast(info)->___data; } void const *value_ptr(holder_info const *info) noexcept { - return reinterpret_cast( - ::LIBIMP::round_up(reinterpret_cast(info + 1), alignof(std::max_align_t))); + if (info == nullptr) return nullptr; + return &static_cast(info)->___data; } -class holder_info_ptr { +/// \class holder_data_builder +/// \brief The data pointer builder. +class holder_data_builder { allocator const &alloc_; - holder_info **pptr_; + holder_data **pptr_; std::size_t size_; public: - holder_info_ptr(allocator const &alloc, holder_info *(&ptr), std::size_t sz) noexcept + holder_data_builder(allocator const &alloc, holder_data *(&ptr), std::size_t sz) noexcept : alloc_(alloc) , pptr_ (&ptr) - , size_ (::LIBIMP::round_up(sizeof(holder_info), alignof(std::max_align_t)) + sz) { - *pptr_ = static_cast(alloc_.allocate(size_)); + , size_ (detail::full_sizeof<::LIBIMP::byte>(sz)) { + *pptr_ = static_cast(alloc_.allocate(size_)); } - ~holder_info_ptr() noexcept { + ~holder_data_builder() noexcept { if (pptr_ == nullptr) return; alloc_.deallocate(*pptr_, size_); *pptr_ = nullptr; @@ -284,11 +294,11 @@ class holder_info_ptr { return (pptr_ != nullptr) && (*pptr_ != nullptr); } - holder_info *operator->() const noexcept { + holder_data *operator->() const noexcept { return *pptr_; } - holder_info_ptr &operator=(holder_info const &rhs) noexcept { + holder_data_builder &operator=(holder_data const &rhs) noexcept { if (!*this) return *this; **pptr_ = rhs; return *this; @@ -308,7 +318,7 @@ class holder_info_ptr { template <> class holder : public holder_base { - detail::holder_info info_; + detail::holder_data info_; public: template @@ -385,7 +395,7 @@ class holder : public holder_base { template <> class holder : public holder_base { - detail::holder_info *info_ptr_; + detail::holder_data *info_ptr_; public: holder() noexcept @@ -397,7 +407,7 @@ class holder : public holder_base { std::enable_if_t = true> holder(allocator const &alloc, ::LIBIMP::types, std::size_t n) : holder() { LIBIMP_LOG_("holder"); - detail::holder_info_ptr info_p{alloc, info_ptr_, sizeof(Value) * n}; + detail::holder_data_builder info_p{alloc, info_ptr_, sizeof(Value) * n}; if (!info_p) { log.error("The destination information-pointer failed to be constructed." " type size = ", sizeof(Value), @@ -452,7 +462,7 @@ class holder : public holder_base { LIBIMP_LOG_(); auto *des = ::LIBIMP::construct(p); if (!valid()) return; - detail::holder_info_ptr info_p{alloc, des->info_ptr_, this->sizeof_type() * this->count()}; + detail::holder_data_builder info_p{alloc, des->info_ptr_, this->sizeof_type() * this->count()}; if (!info_p) { log.error("The destination information-pointer failed to be constructed." " type size = ", this->sizeof_type(), diff --git a/src/libipc/platform/posix/shm_impl.h b/src/libipc/platform/posix/shm_impl.h index 91b09bb7..f335862e 100644 --- a/src/libipc/platform/posix/shm_impl.h +++ b/src/libipc/platform/posix/shm_impl.h @@ -53,8 +53,10 @@ result shm_open_fd(std::string const &name, mode::type type) noexcept { log.error("name is empty."); return std::make_error_code(std::errc::invalid_argument); } - - /// \brief Open the object for read-write access. + // For portable use, a shared memory object should be identified by name of the form /somename. + // see: https://man7.org/linux/man-pages/man3/shm_open.3.html + std::string op_name = '/' + name; + // Open the object for read-write access. int flag = O_RDWR; switch (type) { case mode::open: @@ -73,13 +75,13 @@ result shm_open_fd(std::string const &name, mode::type type) noexcept { return std::make_error_code(std::errc::invalid_argument); } - /// \brief Create/Open POSIX shared memory bject - int fd = ::shm_open(name.c_str(), flag, S_IRUSR | S_IWUSR | - S_IRGRP | S_IWGRP | - S_IROTH | S_IWOTH); + // Create/Open POSIX shared memory bject. + int fd = ::shm_open(op_name.c_str(), flag, S_IRUSR | S_IWUSR | + S_IRGRP | S_IWGRP | + S_IROTH | S_IWOTH); if (fd == posix::failed) { auto err = sys::error(); - log.error("failed: shm_open(name = ", name, + log.error("failed: shm_open(name = ", op_name, ", type = ", type, "). error = ", err); return err; diff --git a/src/libipc/platform/win/mmap_impl.h b/src/libipc/platform/win/mmap_impl.h index a28ef1b0..e911a98f 100644 --- a/src/libipc/platform/win/mmap_impl.h +++ b/src/libipc/platform/win/mmap_impl.h @@ -72,7 +72,7 @@ result mmap_open(std::string const &file, std::size_t size, mode::type t return std::make_error_code(std::errc::invalid_argument); } - /// \brief Opens a named file mapping object. + // Opens a named file mapping object. auto try_open = [&]() -> result { HANDLE h = ::OpenFileMapping(FILE_MAP_ALL_ACCESS, FALSE, t_name.c_str()); if (h == NULL) { @@ -83,7 +83,7 @@ result mmap_open(std::string const &file, std::size_t size, mode::type t return h; }; - /// \brief Creates or opens a named or unnamed file mapping object for a specified file. + // Creates or opens a named or unnamed file mapping object for a specified file. auto try_create = [&]() -> result { HANDLE h = ::CreateFileMapping(INVALID_HANDLE_VALUE, detail::get_sa(), PAGE_READWRITE | SEC_COMMIT, /// \remark dwMaximumSizeHigh always 0 here. diff --git a/src/libipc/shm.cpp b/src/libipc/shm.cpp index 40ef31d8..4b6d54d9 100644 --- a/src/libipc/shm.cpp +++ b/src/libipc/shm.cpp @@ -57,7 +57,7 @@ shared_memory::shared_memory(shared_memory &&other) noexcept : shm_(std::exchange(other.shm_, nullptr)) {} shared_memory &shared_memory::operator=(shared_memory &&rhs) & noexcept { - this->shm_ = std::exchange(rhs.shm_, nullptr); + shared_memory(std::move(rhs)).swap(*this); return *this; } diff --git a/src/libpmr/memory_resource.cpp b/src/libpmr/memory_resource.cpp index 726fc02c..7231154f 100644 --- a/src/libpmr/memory_resource.cpp +++ b/src/libpmr/memory_resource.cpp @@ -4,6 +4,7 @@ #include "libimp/detect_plat.h" #include "libimp/system.h" #include "libimp/log.h" +#include "libimp/aligned.h" #include "libpmr/memory_resource.h" @@ -52,7 +53,8 @@ void *new_delete_resource::allocate(std::size_t bytes, std::size_t alignment) no } #if defined(LIBIMP_CPP_17) /// \see https://en.cppreference.com/w/cpp/memory/c/aligned_alloc - return std::aligned_alloc(alignment, bytes); + /// \remark The size parameter must be an integral multiple of alignment. + return std::aligned_alloc(alignment, ::LIBIMP::round_up(bytes, alignment)); #else if (alignment <= alignof(std::max_align_t)) { /// \see https://en.cppreference.com/w/cpp/memory/c/malloc diff --git a/test/imp/test_imp_byte.cpp b/test/imp/test_imp_byte.cpp index c853c2ec..a21daeef 100644 --- a/test/imp/test_imp_byte.cpp +++ b/test/imp/test_imp_byte.cpp @@ -8,19 +8,23 @@ TEST(byte, construct) { { imp::byte b; + SUCCEED(); + } + { + imp::byte b{}; EXPECT_EQ(int(b), 0); } { - imp::byte b {123}; + imp::byte b{123}; EXPECT_EQ(int(b), 123); } { - imp::byte b {65535}; + imp::byte b{65535}; EXPECT_EQ(int(b), 255); EXPECT_EQ(std::int8_t(b), -1); } { - imp::byte b {65536}; + imp::byte b{65536}; EXPECT_EQ(int(b), 0); } } diff --git a/test/pmr/test_pmr_small_storage.cpp b/test/pmr/test_pmr_small_storage.cpp index 88914ab8..0f575134 100644 --- a/test/pmr/test_pmr_small_storage.cpp +++ b/test/pmr/test_pmr_small_storage.cpp @@ -41,14 +41,18 @@ TEST(small_storage, holder_copy_move_construct) { EXPECT_FALSE((std::is_move_assignable>::value)); } -TEST(small_storage, holder_copy_move) { - struct foo { - int i; - foo(int i) : i(i) {} - foo(foo const &rhs) : i(rhs.i) {} - foo(foo &&rhs) : i(std::exchange(rhs.i, 0)) {} - }; +namespace { +struct foo { + int i; + foo(int i) : i(i) {} + foo(foo const &rhs) : i(rhs.i) {} + foo(foo &&rhs) : i(std::exchange(rhs.i, 0)) {} +}; + +} // namespace + +TEST(small_storage, holder_copy_move_object_stack) { pmr::allocator alc; pmr::holder h1(alc, 1); pmr::holder h2, h3; @@ -61,7 +65,10 @@ TEST(small_storage, holder_copy_move) { h1.destroy(alc); h2.destroy(alc); h3.destroy(alc); +} +TEST(small_storage, holder_copy_move_object_heap) { + pmr::allocator alc; pmr::holder h4(alc, 1); pmr::holder h5, h6; h4.copy_to(alc, &h5); @@ -73,17 +80,20 @@ TEST(small_storage, holder_copy_move) { h4.destroy(alc); h5.destroy(alc); h6.destroy(alc); +} +TEST(small_storage, holder_copy_move_array_stack) { + pmr::allocator alc; void *ph1 = std::malloc(pmr::holder::full_sizeof(10)); void *ph2 = std::malloc(pmr::holder::full_sizeof(10)); void *ph3 = std::malloc(pmr::holder::full_sizeof(10)); auto *h7 = ::new (ph1) pmr::holder(alc, ::LIBIMP::types{}, 10); - auto *h8 = ::new (ph2) pmr::holder; - auto *h9 = ::new (ph2) pmr::holder; - h7->copy_to(alc, h8); + auto *h8 = static_cast *>(ph2); + auto *h9 = static_cast *>(ph3); + h7->copy_to(alc, ph2); EXPECT_EQ(h7->count(), 10); EXPECT_EQ(h8->count(), 10); - h7->move_to(alc, h9); + h7->move_to(alc, ph3); EXPECT_EQ(h7->count(), 0); EXPECT_EQ(h9->count(), 10); h7->destroy(alc); @@ -92,7 +102,10 @@ TEST(small_storage, holder_copy_move) { std::free(ph1); std::free(ph2); std::free(ph3); +} +TEST(small_storage, holder_copy_move_array_heap) { + pmr::allocator alc; pmr::holder h10(alc, ::LIBIMP::types{}, 10); pmr::holder h11, h12; h10.copy_to(alc, &h11); @@ -110,14 +123,14 @@ TEST(small_storage, sizeof) { EXPECT_EQ(sizeof(pmr::holder_null), sizeof(void *)); EXPECT_EQ(sizeof(pmr::holder), sizeof(void *) + imp::round_up(sizeof(int), alignof(void *))); EXPECT_EQ(sizeof(pmr::holder), sizeof(void *) + sizeof(void *)); - EXPECT_EQ(sizeof(pmr::holder), sizeof(void *) + sizeof(pmr::detail::holder_info)); + EXPECT_EQ(sizeof(pmr::holder), imp::round_up(sizeof(void *) + sizeof(pmr::detail::holder_data), alignof(std::max_align_t))); EXPECT_EQ(sizeof(pmr::holder), sizeof(void *) + sizeof(void *)); // pmr::small_storage<4> s1; - EXPECT_TRUE(sizeof(pmr::small_storage<16>) > 16); - EXPECT_TRUE(sizeof(pmr::small_storage<64>) > 64); - EXPECT_TRUE(sizeof(pmr::small_storage<512>) > 512); - EXPECT_TRUE(sizeof(pmr::small_storage<4096>) > 4096); + EXPECT_GT(sizeof(pmr::small_storage<16>) , 16); + EXPECT_GT(sizeof(pmr::small_storage<64>) , 64); + EXPECT_GT(sizeof(pmr::small_storage<512>) , 512); + EXPECT_GT(sizeof(pmr::small_storage<4096>), 4096); } TEST(small_storage, construct) {