From: JINMEI Tatuya <jinmei@isc.org>
Date: Fri, 10 May 2013 23:39:32 +0000 (-0700)
Subject: [2836-2] make sure remapping boost segment even if grow() fails.
X-Git-Tag: bind10-1.2.0beta1-release~457^2~26
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=3a6ae66c2a737475ed55fb28cd3cd49b49e7295e;p=thirdparty%2Fkea.git

[2836-2] make sure remapping boost segment even if grow() fails.

otherwise, subsequent cleanup to be exception safe will cause a crash.
and, if the remap fails simply abort(); there's no hope to recover from
this situation reasonably.  and clarify in shrinkToFit() why it behaves
differently.
---

diff --git a/src/lib/util/memory_segment_mapped.cc b/src/lib/util/memory_segment_mapped.cc
index 5b9acdf684..b74fbd4f11 100644
--- a/src/lib/util/memory_segment_mapped.cc
+++ b/src/lib/util/memory_segment_mapped.cc
@@ -140,16 +140,21 @@ struct MemorySegmentMapped::Impl {
         const size_t new_size = prev_size * 2;
         assert(new_size > prev_size);
 
-        if (!BaseSegment::grow(filename_.c_str(), new_size - prev_size)) {
-            throw std::bad_alloc();
-        }
-
+        const bool grown = BaseSegment::grow(filename_.c_str(),
+                                             new_size - prev_size);
+
+        // Remap the file, whether or not grow() succeeded.  this should
+        // normally succeed(*), but it's not 100% guaranteed.  We abort
+        // if it fails (see the method description in the header file).
+        // (*) Although it's not formally documented, the implementation
+        // of grow() seems to provide strong guarantee, i.e, if it fails
+        // the underlying file can be used with the previous size.
         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.
             base_sgmt_.reset(new BaseSegment(open_only, filename_.c_str()));
-        } catch (const boost::interprocess::interprocess_exception& ex) {
+        } catch (...) {
+            abort();
+        }
+        if (!grown) {
             throw std::bad_alloc();
         }
     }
@@ -350,7 +355,12 @@ MemorySegmentMapped::shrinkToFit() {
     try {
         // Remap the shrunk 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.
+        // the new segment.  Note that this is different from the case where
+        // reset() after grow() fails.  While the same argument can apply
+        // in theory, it should be less likely that other methods will be
+        // called after shrinkToFit() (and the destructor can still be called
+        // safely), so we give the application an opportunity to handle the
+        // case as gracefully as possible.
         impl_->base_sgmt_.reset(
             new BaseSegment(open_only, impl_->filename_.c_str()));
     } catch (const boost::interprocess::interprocess_exception& ex) {
diff --git a/src/lib/util/memory_segment_mapped.h b/src/lib/util/memory_segment_mapped.h
index fe8a29a66d..c6c79a145e 100644
--- a/src/lib/util/memory_segment_mapped.h
+++ b/src/lib/util/memory_segment_mapped.h
@@ -147,7 +147,14 @@ public:
 
     /// \brief Allocate/acquire a segment of memory.
     ///
-    /// This version can throw \c MemorySegmentGrown.
+    /// This version can throw \c MemorySegmentGrown.  Furthermore, there is
+    /// a very small chance that the object loses its integrity and can't be
+    /// usable in the case where \c MemorySegmentGrown would be thrown.
+    /// In this case, throwing a different exception wouldn't help, because
+    /// an application trying to provide exception safety might then call
+    /// deallocate() or named address APIs on this object, which would simply
+    /// cause a crash.  So, while suboptimal, this method just aborts the
+    /// program in this case, indicating there's no hope to shutdown cleanly.
     ///
     /// This method cannot be called if the segment object is created in the
     /// read-only mode; in that case MemorySegmentError will be thrown.
diff --git a/src/lib/util/tests/memory_segment_mapped_unittest.cc b/src/lib/util/tests/memory_segment_mapped_unittest.cc
index 1d9979de9e..bd37a737e4 100644
--- a/src/lib/util/tests/memory_segment_mapped_unittest.cc
+++ b/src/lib/util/tests/memory_segment_mapped_unittest.cc
@@ -238,11 +238,12 @@ TEST_F(MemorySegmentMappedTest, allocate) {
 
 TEST_F(MemorySegmentMappedTest, badAllocate) {
     // Make the mapped file non-writable; managed_mapped_file::grow() will
-    // fail, resulting in std::bad_alloc
+    // fail, resulting in abort.
     const int ret = chmod(mapped_file, 0444);
     ASSERT_EQ(0, ret);
 
-    EXPECT_THROW(segment_->allocate(DEFAULT_INITIAL_SIZE * 2), std::bad_alloc);
+    EXPECT_DEATH_IF_SUPPORTED(
+        {segment_->allocate(DEFAULT_INITIAL_SIZE * 2);}, "");
 }
 
 // XXX: this test can cause too strong side effect (creating a very large