From: JINMEI Tatuya Date: Sat, 13 Apr 2013 06:05:24 +0000 (-0700) Subject: [2831] use boost file lock to detect any violation of read-write conflicts X-Git-Tag: bind10-1.2.0beta1-release~457^2~45 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=0f6cfdfd2a611d2a440fcb01596c4f6173e9ee04;p=thirdparty%2Fkea.git [2831] use boost file lock to detect any violation of read-write conflicts this is done best-effort basis. update doc accordingly. --- diff --git a/src/lib/util/memory_segment_mapped.cc b/src/lib/util/memory_segment_mapped.cc index 7ea9e8a5b3..e2ac944c0e 100644 --- a/src/lib/util/memory_segment_mapped.cc +++ b/src/lib/util/memory_segment_mapped.cc @@ -22,12 +22,25 @@ #include #include #include +#include #include #include #include -using namespace boost::interprocess; +// boost::interprocess namespace is big and can cause unexpected import +// (e.g., it has "read_only"), so it's safer to be specific for shortcuts. +using boost::interprocess::basic_managed_mapped_file; +using boost::interprocess::rbtree_best_fit; +using boost::interprocess::null_mutex_family; +using boost::interprocess::iset_index; +using boost::interprocess::create_only_t; +using boost::interprocess::create_only; +using boost::interprocess::open_or_create_t; +using boost::interprocess::open_or_create; +using boost::interprocess::open_read_only; +using boost::interprocess::open_only; +using boost::interprocess::offset_ptr; namespace isc { namespace util { @@ -47,27 +60,70 @@ typedef basic_managed_mapped_file BaseSegment; struct MemorySegmentMapped::Impl { - // Constructor for create-only (and read-write) mode + // Constructor for create-only (and read-write) mode. this case is + // tricky because we want to remove any existing file but we also want + // to detect possible conflict with other readers or writers using + // file lock. Impl(const std::string& filename, create_only_t, size_t initial_size) : - read_only_(false), filename_(filename), - base_sgmt_(new BaseSegment(create_only, filename.c_str(), - initial_size)) - {} + read_only_(false), filename_(filename) + { + try { + // First, try opening it in boost create_only mode; it fails if + // the file exists (among other reasons). + base_sgmt_.reset(new BaseSegment(create_only, filename.c_str(), + initial_size)); + } catch (const boost::interprocess::interprocess_exception& ex) { + // We assume this is because the file exists; otherwise creating + // file_lock would fail with interprocess_exception, and that's + // what we want here (we wouldn't be able to create a segment + // anyway). + lock_.reset(new boost::interprocess::file_lock(filename.c_str())); + + // Confirm there's no other reader or writer, and then release + // the lock before we remove the file; there's a chance of race + // here, but this check doesn't intend to guarantee 100% safety + // and so it should be okay. + checkWriter(); + lock_.reset(); + + // now remove the file (if it happens to have been delete, this + // will be no-op), then re-open it with create_only. this time + // it should succeed, and if it fails again, that's fatal for this + // constructor. + boost::interprocess::file_mapping::remove(filename.c_str()); + base_sgmt_.reset(new BaseSegment(create_only, filename.c_str(), + initial_size)); + } + + // confirm there's no other user and there won't either. + lock_.reset(new boost::interprocess::file_lock(filename.c_str())); + checkWriter(); + } // Constructor for open-or-write (and read-write) mode Impl(const std::string& filename, open_or_create_t, size_t initial_size) : read_only_(false), filename_(filename), base_sgmt_(new BaseSegment(open_or_create, filename.c_str(), - initial_size)) - {} + initial_size)), + lock_(new boost::interprocess::file_lock(filename.c_str())) + { + checkWriter(); + } // Constructor for existing segment, either read-only or read-write Impl(const std::string& filename, bool read_only) : read_only_(read_only), filename_(filename), - base_sgmt_(read_only ? + base_sgmt_(read_only_ ? new BaseSegment(open_read_only, filename.c_str()) : - new BaseSegment(open_only, filename.c_str())) - {} + new BaseSegment(open_only, filename.c_str())), + lock_(new boost::interprocess::file_lock(filename.c_str())) + { + if (read_only_) { + checkReader(); + } else { + checkWriter(); + } + } // Internal helper to grow the underlying mapped segment. void growSegment() { @@ -105,6 +161,30 @@ struct MemorySegmentMapped::Impl { // actual Boost implementation of mapped segment. boost::scoped_ptr base_sgmt_; + +private: + // helper methods and member to detect any reader-writer conflict at + // the time of construction using an advisory file lock. The lock will + // be held throughout the lifetime of the object and will be released + // automatically. + + void checkReader() { + if (!lock_->try_lock_sharable()) { + isc_throw(MemorySegmentOpenError, + "mapped memory segment can't be opened as read-only " + "with a writer process"); + } + } + + void checkWriter() { + if (!lock_->try_lock()) { + isc_throw(MemorySegmentOpenError, + "mapped memory segment can't be opened as read-write " + "with other reader or writer processes"); + } + } + + boost::scoped_ptr lock_; }; MemorySegmentMapped::MemorySegmentMapped(const std::string& filename) : @@ -132,8 +212,6 @@ MemorySegmentMapped::MemorySegmentMapped(const std::string& filename, impl_ = new Impl(filename, open_or_create, initial_size); break; case CREATE_ONLY: - // Remove any existing file then create a new one - file_mapping::remove(filename.c_str()); impl_ = new Impl(filename, create_only, initial_size); break; default: diff --git a/src/lib/util/memory_segment_mapped.h b/src/lib/util/memory_segment_mapped.h index 142d2b7924..2ca02a5200 100644 --- a/src/lib/util/memory_segment_mapped.h +++ b/src/lib/util/memory_segment_mapped.h @@ -35,13 +35,14 @@ namespace util { /// In the read-write mode the object can allocate or deallocate memory /// from the mapped image, and the owner process can change the content. /// -/// This class does not provide any synchronization mechanism between -/// read-only and read-write objects that share the same mapped image; -/// in fact, the expected usage is the application (or a system of related -/// processes) ensures there's at most one read-write object and if there's -/// such an object, no read-only object shares the image. If an application -/// uses this class beyond that expectation, it's the application's -/// responsibility to provide necessary synchronization between the processes. +/// Multiple processes can open multiple segments for the same file in +/// read-only mode at the same time. But there shouldn't be more than +/// one process that opens segments for the same file in read-write mode +/// at the same time. Likewise, if one process opens a segment for a file +/// there shouldn't be any other process that opens a segment for the file +/// in read-only mode. This class tries to detect any violation of this +/// restriction, but this does not intend to provide 100% safety. It's +/// generally the user's responsibility to ensure this condition. class MemorySegmentMapped : boost::noncopyable, public MemorySegment { public: /// \brief The default value of the mapped file size when newly created. @@ -74,7 +75,8 @@ public: /// exception will be thrown. /// /// \throw MemorySegmentOpenError The given file does not exist, is not - /// readable, or not valid mappable segment. + /// readable, or not valid mappable segment. Or there is another process + /// that has already opened a segment for the file. /// \throw std::bad_alloc (rare case) internal resource allocation /// failure. /// @@ -106,6 +108,10 @@ public: /// create or initialization fails, \c MemorySegmentOpenError exception /// will be thrown. /// + /// This constructor also throws \c MemorySegmentOpenError when it + /// detects violation of the restriction on the mixed open of read-only + /// and read-write mode (see the class description). + /// /// When initial_size is specified but is too small (including a value of /// 0), the underlying Boost library will reject it, and this constructor /// throws \c MemorySegmentOpenError exception. The Boost documentation diff --git a/src/lib/util/tests/memory_segment_mapped_unittest.cc b/src/lib/util/tests/memory_segment_mapped_unittest.cc index fcb55e7007..111cb33dcf 100644 --- a/src/lib/util/tests/memory_segment_mapped_unittest.cc +++ b/src/lib/util/tests/memory_segment_mapped_unittest.cc @@ -56,6 +56,25 @@ const MemorySegmentMapped::OpenMode CREATE_ONLY = const char* const mapped_file = TEST_DATA_BUILDDIR "/test.mapped"; const size_t DEFAULT_INITIAL_SIZE = 32 * 1024; // intentionally hardcoded +// A simple RAII-style wrapper for a pipe. Several tests in this file use +// pipes, so this helper will be useful. +class PipeHolder { +public: + PipeHolder() { + if (pipe(fds_) == -1) { + isc_throw(isc::Unexpected, "pipe failed"); + } + } + ~PipeHolder() { + close(fds_[0]); + close(fds_[1]); + } + int getReadFD() const { return (fds_[0]); } + int getWriteFD() const { return (fds_[1]); } +private: + int fds_[2]; +}; + class MemorySegmentMappedTest : public ::testing::Test { protected: MemorySegmentMappedTest() { @@ -102,6 +121,7 @@ TEST_F(MemorySegmentMappedTest, createAndModify) { // re-open it in read-write mode, but don't try to create it // this time. + segment_.reset(); // make sure close is first. segment_.reset(new MemorySegmentMapped(mapped_file, OPEN_FOR_WRITE)); } } @@ -113,6 +133,7 @@ TEST_F(MemorySegmentMappedTest, createWithSize) { // the size is actually the specified one. const size_t new_size = 64 * 1024; EXPECT_NE(new_size, segment_->getSize()); + segment_.reset(); segment_.reset(new MemorySegmentMapped(mapped_file, OPEN_OR_CREATE, new_size)); EXPECT_EQ(new_size, segment_->getSize()); @@ -263,22 +284,6 @@ checkNamedData(const std::string& name, const std::vector& data, ASSERT_TRUE(dp); EXPECT_EQ(0, std::memcmp(dp, &data[0], data.size())); - // Open a separate read-only segment and checks the same named data - // Since the mapped space should be different, the resulting bare address - // from getNamedAddress should also be different, but it should actually - // point to the same data. - // Note: it's mostly violation of the API assumption to open read-only - // and read-write segments at the same time, but unless we modify the - // segment throughout the lifetime of the read-only segment, it should - // work. - scoped_ptr segment_ro( - new MemorySegmentMapped(mapped_file)); - void* dp2 = segment_ro->getNamedAddress(name.c_str()); - ASSERT_NE(static_cast(NULL), dp2); - EXPECT_NE(dp, dp2); - EXPECT_EQ(0, std::memcmp(dp2, &data[0], data.size())); - segment_ro.reset(); - if (delete_after_check) { sgmt.deallocate(dp, data.size()); sgmt.clearNamedAddress(name.c_str()); @@ -384,16 +389,16 @@ TEST_F(MemorySegmentMappedTest, multiProcess) { segment_.reset(); // Spawn another process and have it open and read the same data. - int pipes[2]; - EXPECT_EQ(0, pipe(pipes)); + PipeHolder pipe_to_child; + PipeHolder pipe_to_parent; const pid_t child_pid = fork(); ASSERT_NE(-1, child_pid); if (child_pid == 0) { // child: wait until the parent has opened the read-only segment. char from_parent; - EXPECT_EQ(1, read(pipes[0], &from_parent, sizeof(from_parent))); + EXPECT_EQ(1, read(pipe_to_child.getReadFD(), &from_parent, + sizeof(from_parent))); EXPECT_EQ(0, from_parent); - close(pipes[0]); MemorySegmentMapped sgmt(mapped_file); void* ptr_child = sgmt.getNamedAddress("test address"); @@ -404,9 +409,8 @@ TEST_F(MemorySegmentMappedTest, multiProcess) { // tell the parent whether it succeeded. 0 means it did, // 0xff means it failed. const char ok = (val == 424242) ? 0 : 0xff; - EXPECT_EQ(1, write(pipes[1], &ok, sizeof(ok))); + EXPECT_EQ(1, write(pipe_to_parent.getWriteFD(), &ok, sizeof(ok))); } - close(pipes[1]); exit(0); } // parent: open another read-only segment, then tell the child to open @@ -416,12 +420,11 @@ TEST_F(MemorySegmentMappedTest, multiProcess) { ASSERT_TRUE(ptr); EXPECT_EQ(424242, *static_cast(ptr)); const char some_data = 0; - EXPECT_EQ(1, write(pipes[1], &some_data, sizeof(some_data))); - close(pipes[1]); + EXPECT_EQ(1, write(pipe_to_child.getWriteFD(), &some_data, + sizeof(some_data))); // wait for the completion of the child and checks the result. - EXPECT_EQ(0, parentReadState(pipes[0])); - close(pipes[0]); + EXPECT_EQ(0, parentReadState(pipe_to_parent.getReadFD())); } TEST_F(MemorySegmentMappedTest, nullDeallocate) { @@ -453,25 +456,12 @@ TEST_F(MemorySegmentMappedTest, shrink) { } TEST_F(MemorySegmentMappedTest, violateReadOnly) { - // If the segment is opened in the read-only mode, modification - // attempts are prohibited. When detectable it must result in an - // exception. - EXPECT_THROW(MemorySegmentMapped(mapped_file).allocate(16), - MemorySegmentError); - // allocation that would otherwise require growing the segment; permission - // check should be performed before that. - EXPECT_THROW(MemorySegmentMapped(mapped_file). - allocate(DEFAULT_INITIAL_SIZE * 2), MemorySegmentError); - EXPECT_THROW(MemorySegmentMapped(mapped_file).setNamedAddress("test", - NULL), - MemorySegmentError); - EXPECT_THROW(MemorySegmentMapped(mapped_file).clearNamedAddress("test"), - MemorySegmentError); - EXPECT_THROW(MemorySegmentMapped(mapped_file).shrinkToFit(), - MemorySegmentError); - + // Create a named address for the tests below, then reset the writer + // segment so that it won't fail for different reason (i.e., read-write + // conflict). void* ptr = segment_->allocate(sizeof(uint32_t)); segment_->setNamedAddress("test address", ptr); + segment_.reset(); // Attempts to modify memory from the read-only segment directly // will result in a crash. @@ -484,8 +474,23 @@ TEST_F(MemorySegmentMappedTest, violateReadOnly) { }, ""); } - EXPECT_THROW(MemorySegmentMapped(mapped_file).deallocate(ptr, 4), + // If the segment is opened in the read-only mode, modification + // attempts are prohibited. When detectable it must result in an + // exception. + MemorySegmentMapped segment_ro(mapped_file); + ptr = segment_ro.getNamedAddress("test address"); + EXPECT_NE(static_cast(NULL), ptr); + + EXPECT_THROW(segment_ro.deallocate(ptr, 4), MemorySegmentError); + + EXPECT_THROW(segment_ro.allocate(16), MemorySegmentError); + // allocation that would otherwise require growing the segment; permission + // check should be performed before that. + EXPECT_THROW(segment_ro.allocate(DEFAULT_INITIAL_SIZE * 2), MemorySegmentError); + EXPECT_THROW(segment_ro.setNamedAddress("test", NULL), MemorySegmentError); + EXPECT_THROW(segment_ro.clearNamedAddress("test"), MemorySegmentError); + EXPECT_THROW(segment_ro.shrinkToFit(), MemorySegmentError); } TEST_F(MemorySegmentMappedTest, getCheckSum) { @@ -505,4 +510,102 @@ TEST_F(MemorySegmentMappedTest, getCheckSum) { EXPECT_EQ(old_cksum + 1, segment_->getCheckSum()); } +// Mode of opening segments in the tests below. +enum TestOpenMode { + READER = 0, + WRITER_FOR_WRITE, + WRITER_OPEN_OR_CREATE, + WRITER_CREATE_ONLY +}; + +// A shortcut to attempt to open a specified type of segment (generally +// expecting it to fail) +void +setSegment(TestOpenMode mode, scoped_ptr& sgmt_ptr) { + switch (mode) { + case READER: + sgmt_ptr.reset(new MemorySegmentMapped(mapped_file)); + break; + case WRITER_FOR_WRITE: + sgmt_ptr.reset(new MemorySegmentMapped(mapped_file, OPEN_FOR_WRITE)); + break; + case WRITER_OPEN_OR_CREATE: + sgmt_ptr.reset(new MemorySegmentMapped(mapped_file, OPEN_OR_CREATE)); + break; + case WRITER_CREATE_ONLY: + sgmt_ptr.reset(new MemorySegmentMapped(mapped_file, CREATE_ONLY)); + break; + } +} + +// Common logic for conflictReaderWriter test. The segment opened in the +// parent process will prevent the segment in the child from being used. +void +conflictCheck(TestOpenMode parent_mode, TestOpenMode child_mode) { + PipeHolder pipe_to_child; + PipeHolder pipe_to_parent; + const pid_t child_pid = fork(); + ASSERT_NE(-1, child_pid); + + if (child_pid == 0) { + char ch; + EXPECT_EQ(1, read(pipe_to_child.getReadFD(), &ch, sizeof(ch))); + + ch = 0; // 0 = open success, 1 = fail + try { + scoped_ptr sgmt; + setSegment(child_mode, sgmt); + EXPECT_EQ(1, write(pipe_to_parent.getWriteFD(), &ch, sizeof(ch))); + } catch (const MemorySegmentOpenError&) { + ch = 1; + EXPECT_EQ(1, write(pipe_to_parent.getWriteFD(), &ch, sizeof(ch))); + } + exit(0); + } + + // parent: open a segment, then tell the child to open its own segment of + // the specified type. + scoped_ptr sgmt; + setSegment(parent_mode, sgmt); + const char some_data = 0; + EXPECT_EQ(1, write(pipe_to_child.getWriteFD(), &some_data, + sizeof(some_data))); + + // wait for the completion of the child and checks the result. open at + // the child side should fail, so the parent should get the value of 1. + EXPECT_EQ(1, parentReadState(pipe_to_parent.getReadFD())); +} + +TEST_F(MemorySegmentMappedTest, conflictReaderWriter) { + // Test using fork() doesn't work well on valgrind + if (isc::util::unittests::runningOnValgrind()) { + return; + } + + // Below, we check all combinations of conflicts between reader and writer + // will fail. We first make sure there's no other reader or writer. + segment_.reset(); + + // reader opens segment, then writer (OPEN_FOR_WRITE) tries to open + conflictCheck(READER, WRITER_FOR_WRITE); + // reader opens segment, then writer (OPEN_OR_CREATE) tries to open + conflictCheck(READER, WRITER_OPEN_OR_CREATE); + // reader opens segment, then writer (CREATE_ONLY) tries to open + conflictCheck(READER, WRITER_CREATE_ONLY); + + // writer (OPEN_FOR_WRITE) opens a segment, then reader tries to open + conflictCheck(WRITER_FOR_WRITE, READER); + // writer (OPEN_OR_CREATE) opens a segment, then reader tries to open + conflictCheck(WRITER_OPEN_OR_CREATE, READER); + // writer (CREATE_ONLY) opens a segment, then reader tries to open + conflictCheck(WRITER_CREATE_ONLY, READER); + + // writer opens segment, then another writer (OPEN_FOR_WRITE) tries to open + conflictCheck(WRITER_FOR_WRITE, WRITER_FOR_WRITE); + // writer opens segment, then another writer (OPEN_OR_CREATE) tries to open + conflictCheck(WRITER_FOR_WRITE, WRITER_OPEN_OR_CREATE); + // writer opens segment, then another writer (CREATE_ONLY) tries to open + conflictCheck(WRITER_FOR_WRITE, WRITER_CREATE_ONLY); +} + }