[2831] some cleanups

src/lib/util/memory_segment_mapped.cc

@@ -83,14 +83,19 @@ MemorySegmentMapped::~MemorySegmentMapped() {
 
 void*
 MemorySegmentMapped::allocate(size_t size) {
-    void* ptr = impl_->base_sgmt_->allocate(size, std::nothrow);
-    if (ptr) {
-        return (ptr);
+    // We explicitly check the free memory size; it appears
+    // managed_mapped_file::allocate() could incorrectly return a seemingly
+    // valid pointer for some very large requested size.
+    if (impl_->base_sgmt_->get_free_memory() >= size) {
+        void* ptr = impl_->base_sgmt_->allocate(size, std::nothrow);
+        if (ptr) {
+            return (ptr);
+        }
     }
 
     // Grow the mapped segment doubling the size until we have sufficient
     // free memory in the revised segment for the requested size.
-    while (impl_->base_sgmt_->get_free_memory() < size) {
+    do {
         // We first need to unmap it before calling grow().
         const size_t prev_size = impl_->base_sgmt_->get_size();
         impl_->base_sgmt_.reset();
@@ -98,12 +103,22 @@ MemorySegmentMapped::allocate(size_t size) {
         const size_t new_size = prev_size * 2;
         assert(new_size != 0); // assume grow fails before size overflow
 
-        // TBD error handling
-        managed_mapped_file::grow(impl_->filename_.c_str(),
-                                  new_size - prev_size);
-        impl_->base_sgmt_.reset(
-            new managed_mapped_file(open_only, impl_->filename_.c_str()));
-    }
+        if (!managed_mapped_file::grow(impl_->filename_.c_str(),
+                                       new_size - prev_size))
+        {
+            throw std::bad_alloc();
+        }
+
+        try {
+            // Remap the grown file; this should succeed, but it's not 100%
+            // guaranteed.  If it fails we treat it as if we fail to create
+            // the new segment.
+            impl_->base_sgmt_.reset(
+                new managed_mapped_file(open_only, impl_->filename_.c_str()));
+        } catch (const boost::interprocess::interprocess_exception& ex) {
+            throw std::bad_alloc();
+        }
+    } while (impl_->base_sgmt_->get_free_memory() < size);
     isc_throw(MemorySegmentGrown, "mapped memory segment grown, size: "
               << impl_->base_sgmt_->get_size() << ", free size: "
               << impl_->base_sgmt_->get_free_memory());

src/lib/util/tests/memory_segment_mapped_unittest.cc

@@ -19,6 +19,10 @@
 #include <boost/interprocess/file_mapping.hpp>
 #include <boost/scoped_ptr.hpp>
 
+#include <cstdlib>
+#include <limits>
+#include <stdexcept>
+
 using namespace isc::util;
 using boost::scoped_ptr;
 
@@ -111,7 +115,7 @@ TEST_F(MemorySegmentMappedTest, allocate) {
     EXPECT_THROW(segment_->allocate(DEFAULT_INITIAL_SIZE + 1),
                  MemorySegmentGrown);
     // The size should have been doubled.
-    EXPECT_EQ(DEFAULT_INITIAL_SIZE * 2,segment_->getSize());
+    EXPECT_EQ(DEFAULT_INITIAL_SIZE * 2, segment_->getSize());
     // In this case it should now succeed.
     void* ptr = segment_->allocate(DEFAULT_INITIAL_SIZE + 1);
     EXPECT_NE(static_cast<void*>(0), ptr);
@@ -121,8 +125,30 @@ TEST_F(MemorySegmentMappedTest, allocate) {
     // Same set of checks, but for a larger size.
     EXPECT_THROW(segment_->allocate(DEFAULT_INITIAL_SIZE * 10),
                  MemorySegmentGrown);
+    // the segment should have grown to the minimum size that could allocate
+    // the given size of memory.
+    EXPECT_EQ(DEFAULT_INITIAL_SIZE * 16, segment_->getSize());
+    // And allocate() should now succeed.
     ptr = segment_->allocate(DEFAULT_INITIAL_SIZE * 10);
     EXPECT_NE(static_cast<void*>(0), ptr);
+
+    // (we'll left the regions created in the file there; the entire file
+    // will be removed at the end of the test)
+}
+
+TEST_F(MemorySegmentMappedTest, badAllocate) {
+    // Make the mapped file non-writable; managed_mapped_file::grow() will
+    // fail, resulting in std::bad_alloc
+    const std::string chmod_cmd = "chmod 444 " + std::string(mapped_file);
+    std::system(chmod_cmd.c_str());
+    EXPECT_THROW(segment_->allocate(DEFAULT_INITIAL_SIZE * 2), std::bad_alloc);
+}
+
+// XXX: this test can cause too strong side effect (creating a very large
+// file), so we disable it by default
+TEST_F(MemorySegmentMappedTest, DISABLED_allocateHuge) {
+    EXPECT_THROW(segment_->allocate(std::numeric_limits<size_t>::max()),
+                 std::bad_alloc);
 }
 
 TEST_F(MemorySegmentMappedTest, DISABLED_basics) {
@@ -159,14 +185,6 @@ TEST_F(MemorySegmentMappedTest, DISABLED_basics) {
 }
 
 /*
-TEST(MemorySegmentLocal, TestTooMuchMemory) {
-    auto_ptr<MemorySegment> segment(new MemorySegmentLocal());
-
-    EXPECT_THROW(segment->allocate(ULONG_MAX), bad_alloc);
-}
-*/
-
-/*
 TEST(MemorySegmentLocal, TestBadDeallocate) {
     auto_ptr<MemorySegment> segment(new MemorySegmentLocal());