[2831] use boost file lock to detect any violation of read-write conflicts

this is done best-effort basis.  update doc accordingly.

src/lib/util/memory_segment_mapped.cc

@@ -22,12 +22,25 @@
 #include <boost/interprocess/managed_mapped_file.hpp>
 #include <boost/interprocess/offset_ptr.hpp>
 #include <boost/interprocess/mapped_region.hpp>
+#include <boost/interprocess/sync/file_lock.hpp>
 
 #include <cassert>
 #include <string>
 #include <new>
 
-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<char,
                                   iset_index> 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<BaseSegment> 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<boost::interprocess::file_lock> 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:

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

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<uint8_t>& 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<MemorySegmentMapped> segment_ro(
-        new MemorySegmentMapped(mapped_file));
-    void* dp2 = segment_ro->getNamedAddress(name.c_str());
-    ASSERT_NE(static_cast<void*>(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<const uint32_t*>(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<void*>(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<MemorySegmentMapped>& 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<MemorySegmentMapped> 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<MemorySegmentMapped> 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);
+}
+
 }