From 955e11bc28ad146be77ea118e5bd58a1282aece4 Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Sat, 22 Feb 2020 17:37:22 +0800 Subject: [PATCH 1/2] [block-cache] fix potential file descriptor leak Encapsulate file descriptor into an object, to ensure that an fd will be closed properly while exception raised, e.g., the block_cache throws exception during the block_manager's construction. --- base/file_utils.cc | 28 ++++++++++++++-------------- base/file_utils.h | 15 ++++++++++++--- block-cache/block_cache.cc | 11 +++++------ block-cache/block_cache.h | 5 +++-- persistent-data/block.h | 7 ++++--- persistent-data/block.tcc | 2 +- thin-provisioning/cache_stream.cc | 2 +- thin-provisioning/cache_stream.h | 2 +- unit-tests/bcache_t.cc | 2 +- 9 files changed, 42 insertions(+), 32 deletions(-) diff --git a/base/file_utils.cc b/base/file_utils.cc index 7883cfe..e4f3722 100644 --- a/base/file_utils.cc +++ b/base/file_utils.cc @@ -39,14 +39,16 @@ namespace { //---------------------------------------------------------------- -int -file_utils::open_file(string const &path, int flags) { - int fd = ::open(path.c_str(), OPEN_FLAGS | flags, DEFAULT_MODE); - if (fd < 0) +file_utils::file_descriptor::file_descriptor(string const &path, int flags) { + fd_ = ::open(path.c_str(), OPEN_FLAGS | flags, DEFAULT_MODE); + if (fd_ < 0) syscall_failed("open", "Note: you cannot run this tool with these options on live metadata."); +} - return fd; +file_utils::file_descriptor::~file_descriptor() { + close(fd_); + fd_ = -1; } bool @@ -76,7 +78,7 @@ file_utils::check_file_exists(string const &file, bool must_be_regular_file) { throw runtime_error("Not a regular file"); } -int +file_utils::file_descriptor file_utils::create_block_file(string const &path, off_t file_size) { if (file_exists(path)) { ostringstream out; @@ -84,16 +86,16 @@ file_utils::create_block_file(string const &path, off_t file_size) { throw runtime_error(out.str()); } - int fd = open_file(path, O_CREAT | O_EXCL | O_RDWR); + file_descriptor fd(path, O_CREAT | O_EXCL | O_RDWR); - int r = ::ftruncate(fd, file_size); + int r = ::ftruncate(fd.fd_, file_size); if (r < 0) syscall_failed("ftruncate"); return fd; } -int +file_utils::file_descriptor file_utils::open_block_file(string const &path, off_t min_size, bool writeable, bool excl) { if (!file_exists(path)) { ostringstream out; @@ -105,7 +107,7 @@ file_utils::open_block_file(string const &path, off_t min_size, bool writeable, if (excl) flags |= O_EXCL; - return open_file(path, flags); + return file_descriptor(path, flags); } uint64_t @@ -146,17 +148,15 @@ file_utils::zero_superblock(std::string const &path) { char *buffer; unsigned const SUPERBLOCK_SIZE = 4096; - int fd = open_block_file(path, SUPERBLOCK_SIZE, true, true); + file_descriptor fd = open_block_file(path, SUPERBLOCK_SIZE, true, true); buffer = reinterpret_cast(aligned_alloc(SUPERBLOCK_SIZE, SUPERBLOCK_SIZE)); if (!buffer) throw runtime_error("out of memory"); memset(buffer, 0, SUPERBLOCK_SIZE); - if (::write(fd, buffer, SUPERBLOCK_SIZE) != SUPERBLOCK_SIZE) + if (::write(fd.fd_, buffer, SUPERBLOCK_SIZE) != SUPERBLOCK_SIZE) throw runtime_error("couldn't zero superblock"); - - ::close(fd); } //---------------------------------------------------------------- diff --git a/base/file_utils.h b/base/file_utils.h index 3edcc9e..686e923 100644 --- a/base/file_utils.h +++ b/base/file_utils.h @@ -8,11 +8,20 @@ //---------------------------------------------------------------- namespace file_utils { - int open_file(std::string const &path, int flags); + struct file_descriptor { + // file_descriptor is movable but not copyable + file_descriptor(file_descriptor &&) = default; + file_descriptor& operator=(file_descriptor &&) = default; + file_descriptor(std::string const &path, int flags); + virtual ~file_descriptor(); + + int fd_; + }; + bool file_exists(std::string const &path); void check_file_exists(std::string const &file, bool must_be_regular_file = true); - int create_block_file(std::string const &path, off_t file_size); - int open_block_file(std::string const &path, off_t min_size, bool writeable, bool excl = true); + file_descriptor create_block_file(std::string const &path, off_t file_size); + file_descriptor open_block_file(std::string const &path, off_t min_size, bool writeable, bool excl = true); uint64_t get_file_length(std::string const &file); void zero_superblock(std::string const &path); } diff --git a/block-cache/block_cache.cc b/block-cache/block_cache.cc index 5a367f4..18a1606 100644 --- a/block-cache/block_cache.cc +++ b/block-cache/block_cache.cc @@ -15,6 +15,7 @@ #include using namespace bcache; +using namespace file_utils; //---------------------------------------------------------------- @@ -290,7 +291,7 @@ block_cache::setup_control_block(block &b) size_t block_size_bytes = block_size_ << SECTOR_SHIFT; memset(cb, 0, sizeof(*cb)); - cb->aio_fildes = fd_; + cb->aio_fildes = fd_.fd_; cb->u.c.buf = b.data_; cb->u.c.offset = block_size_bytes * b.index_; @@ -371,8 +372,9 @@ block_cache::calc_nr_buckets(unsigned nr_blocks) return r; } -block_cache::block_cache(int fd, sector_t block_size, uint64_t on_disk_blocks, size_t mem) - : nr_locked_(0), +block_cache::block_cache(file_descriptor &fd, sector_t block_size, uint64_t on_disk_blocks, size_t mem) + : fd_(fd), + nr_locked_(0), nr_dirty_(0), nr_io_pending_(0), read_hits_(0), @@ -386,7 +388,6 @@ block_cache::block_cache(int fd, sector_t block_size, uint64_t on_disk_blocks, s int r; unsigned nr_cache_blocks = calc_nr_cache_blocks(mem, block_size); - fd_ = fd; block_size_ = block_size; nr_data_blocks_ = on_disk_blocks; nr_cache_blocks_ = nr_cache_blocks; @@ -418,8 +419,6 @@ block_cache::~block_cache() if (aio_context_) io_destroy(aio_context_); - ::close(fd_); - #if 0 std::cerr << "\nblock cache stats\n" << "=================\n" diff --git a/block-cache/block_cache.h b/block-cache/block_cache.h index 650c9bd..3658e89 100644 --- a/block-cache/block_cache.h +++ b/block-cache/block_cache.h @@ -2,6 +2,7 @@ #define BLOCK_CACHE_H #include "base/container_of.h" +#include "base/file_utils.h" #include #include @@ -185,7 +186,7 @@ namespace bcache { //-------------------------------- - block_cache(int fd, sector_t block_size, + block_cache(file_utils::file_descriptor &fd, sector_t block_size, uint64_t max_nr_blocks, size_t mem); ~block_cache(); @@ -247,7 +248,7 @@ namespace bcache { //-------------------------------- - int fd_; + file_utils::file_descriptor &fd_; sector_t block_size_; uint64_t nr_data_blocks_; uint64_t nr_cache_blocks_; diff --git a/persistent-data/block.h b/persistent-data/block.h index 8a0419e..d5127c2 100644 --- a/persistent-data/block.h +++ b/persistent-data/block.h @@ -136,11 +136,12 @@ namespace persistent_data { private: uint64_t choose_cache_size(block_address nr_blocks) const; - int open_or_create_block_file(std::string const &path, off_t file_size, - mode m, bool excl); + file_utils::file_descriptor open_or_create_block_file(std::string const &path, + off_t file_size, + mode m, bool excl); void check(block_address b) const; - int fd_; + file_utils::file_descriptor fd_; mutable block_cache bc_; unsigned superblock_ref_count_; }; diff --git a/persistent-data/block.tcc b/persistent-data/block.tcc index 4d70151..e743360 100644 --- a/persistent-data/block.tcc +++ b/persistent-data/block.tcc @@ -154,7 +154,7 @@ namespace persistent_data { } template - int + file_utils::file_descriptor block_manager::open_or_create_block_file(std::string const &path, off_t file_size, mode m, bool excl) { switch (m) { diff --git a/thin-provisioning/cache_stream.cc b/thin-provisioning/cache_stream.cc index 7ebcd1c..002b6ba 100644 --- a/thin-provisioning/cache_stream.cc +++ b/thin-provisioning/cache_stream.cc @@ -18,7 +18,7 @@ cache_stream::cache_stream(string const &path, // hack because cache uses LRU rather than MRU cache_blocks_((cache_mem / block_size) / 2u), - fd_(file_utils::open_file(path, O_RDONLY | O_EXCL)), + fd_(path, O_RDONLY | O_EXCL), v_(new bcache::noop_validator()), cache_(new block_cache(fd_, block_size / 512, nr_blocks_, cache_mem)), current_index_(0) { diff --git a/thin-provisioning/cache_stream.h b/thin-provisioning/cache_stream.h index ddcbe15..7779281 100644 --- a/thin-provisioning/cache_stream.h +++ b/thin-provisioning/cache_stream.h @@ -38,7 +38,7 @@ namespace thin_provisioning { block_address nr_blocks_; block_address cache_blocks_; - int fd_; + file_utils::file_descriptor fd_; validator::ptr v_; std::unique_ptr cache_; diff --git a/unit-tests/bcache_t.cc b/unit-tests/bcache_t.cc index 67c45e9..cf21539 100644 --- a/unit-tests/bcache_t.cc +++ b/unit-tests/bcache_t.cc @@ -38,7 +38,7 @@ TEST(BCacheTests, cleaned_on_demand) unsigned const NR_BLOCKS = 16; temp_file tmp("bcache_t", 1); - int fd = open(tmp.get_path().c_str(), O_RDWR | O_DIRECT, 0666); + file_utils::file_descriptor fd(tmp.get_path().c_str(), O_RDWR | O_DIRECT); uint64_t bs = 8; block_cache bc(fd, bs, 64, (bs << SECTOR_SHIFT) * NR_BLOCKS); From 57cae67ff138d269032caa1689d79e793c23070b Mon Sep 17 00:00:00 2001 From: Ming-Hung Tsai Date: Sun, 23 Feb 2020 13:00:55 +0800 Subject: [PATCH 2/2] [unit-tests] fix googletest compile flags googletest now requires c++11 compiler --- unit-tests/Makefile.in | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/unit-tests/Makefile.in b/unit-tests/Makefile.in index 46bc347..c464de1 100644 --- a/unit-tests/Makefile.in +++ b/unit-tests/Makefile.in @@ -36,9 +36,9 @@ GMOCK_DEPS=\ lib/libgmock.a: $(GMOCK_DEPS) @echo " [CXX] gtest" @mkdir -p lib - $(V)g++ $(GMOCK_INCLUDES) -I$(GMOCK_DIR)/googletest -c $(GMOCK_DIR)/googletest/src/gtest-all.cc + $(V)g++ $(GMOCK_INCLUDES) -I$(GMOCK_DIR)/googletest -std=c++11 -c $(GMOCK_DIR)/googletest/src/gtest-all.cc @echo " [CXX] gmock" - $(V)g++ $(GMOCK_INCLUDES) -I$(GMOCK_DIR)/googlemock -c $(GMOCK_DIR)/googlemock/src/gmock-all.cc + $(V)g++ $(GMOCK_INCLUDES) -I$(GMOCK_DIR)/googlemock -std=c++11 -c $(GMOCK_DIR)/googlemock/src/gmock-all.cc @echo " [AR] $<" $(V)ar -rv lib/libgmock.a gtest-all.o gmock-all.o > /dev/null 2>&1