BIND 10 trac2836, updated. f4ca083e427788a22a4979a104b517a5a4ef3df9 [2836-2] use managed_mapped_file to cause remap at different addr.
BIND 10 source code commits
bind10-changes at lists.isc.org
Mon May 13 10:47:41 UTC 2013
The branch, trac2836 has been updated
via f4ca083e427788a22a4979a104b517a5a4ef3df9 (commit)
via 3a6ae66c2a737475ed55fb28cd3cd49b49e7295e (commit)
via 7184eb4185f5bd6577dda90e5100c13e238ed8fe (commit)
from 2f220f9ff1a51348ea225d72a75d58d5d81031aa (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
- Log -----------------------------------------------------------------
commit f4ca083e427788a22a4979a104b517a5a4ef3df9
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Fri May 10 16:52:06 2013 -0700
[2836-2] use managed_mapped_file to cause remap at different addr.
the original setup was not really portable and quite dangerous, making
a very large file.
commit 3a6ae66c2a737475ed55fb28cd3cd49b49e7295e
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Fri May 10 16:39:32 2013 -0700
[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.
commit 7184eb4185f5bd6577dda90e5100c13e238ed8fe
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Fri May 10 16:21:37 2013 -0700
[2836-2] make sure flush()ing the base segment before grow/shrink
I was confused at the time I removed it in #2831. these were actually
necessary.
-----------------------------------------------------------------------
Summary of changes:
.../tests/memory/segment_object_holder_unittest.cc | 22 +++++++++++---
src/lib/util/memory_segment_mapped.cc | 30 ++++++++++++++------
src/lib/util/memory_segment_mapped.h | 9 +++++-
.../util/tests/memory_segment_mapped_unittest.cc | 5 ++--
4 files changed, 50 insertions(+), 16 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/lib/datasrc/tests/memory/segment_object_holder_unittest.cc b/src/lib/datasrc/tests/memory/segment_object_holder_unittest.cc
index 77a36e9..1c21766 100644
--- a/src/lib/datasrc/tests/memory/segment_object_holder_unittest.cc
+++ b/src/lib/datasrc/tests/memory/segment_object_holder_unittest.cc
@@ -17,6 +17,8 @@
#include <datasrc/memory/segment_object_holder.h>
+#include <boost/interprocess/managed_mapped_file.hpp>
+
#include <gtest/gtest.h>
using namespace isc::util;
@@ -92,13 +94,25 @@ TEST(SegmentObjectHolderTest, grow) {
// Allocate a bit of memory, to get a unique address
void* mark = segment.allocate(1);
segment.setNamedAddress("mark", mark);
+
+ // We'd like to cause 'mark' will be mapped at a different address on
+ // MemorySegmentGrown; there doesn't seem to be a reliable and safe way
+ // to cause this situation, but opening another mapped region seems to
+ // often work in practice. We use Boost managed_mapped_file directly
+ // to ignore the imposed file lock with MemorySegmentMapped.
+ using boost::interprocess::managed_mapped_file;
+ using boost::interprocess::open_only;
+ managed_mapped_file mapped_sgmt(open_only, mapped_file);
+
// Try allocating bigger and bigger chunks of data until the segment
// actually relocates
size_t alloc_size = 1024;
- while (mark == segment.getNamedAddress("mark")) {
- EXPECT_THROW(allocateUntilGrows(segment, alloc_size),
- MemorySegmentGrown);
- }
+ EXPECT_THROW(allocateUntilGrows(segment, alloc_size), MemorySegmentGrown);
+ // Confirm it's now mapped at a different address.
+ EXPECT_NE(mark, segment.getNamedAddress("mark"))
+ << "portability assumption for the test doesn't hold; "
+ "disable the test by setting env variable GTEST_FILTER to "
+ "'-SegmentObjectHolderTest.grow'";
mark = segment.getNamedAddress("mark");
segment.clearNamedAddress("mark");
segment.deallocate(mark, 1);
diff --git a/src/lib/util/memory_segment_mapped.cc b/src/lib/util/memory_segment_mapped.cc
index e2ac944..b74fbd4 100644
--- a/src/lib/util/memory_segment_mapped.cc
+++ b/src/lib/util/memory_segment_mapped.cc
@@ -129,6 +129,7 @@ struct MemorySegmentMapped::Impl {
void growSegment() {
// We first need to unmap it before calling grow().
const size_t prev_size = base_sgmt_->get_size();
+ base_sgmt_->flush();
base_sgmt_.reset();
// Double the segment size. In theory, this process could repeat
@@ -139,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();
}
}
@@ -341,14 +347,20 @@ MemorySegmentMapped::shrinkToFit() {
return;
}
- // First, (unmap and) close the underlying file.
+ // First, unmap the underlying file.
+ impl_->base_sgmt_->flush();
impl_->base_sgmt_.reset();
BaseSegment::shrink_to_fit(impl_->filename_.c_str());
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 fe8a29a..c6c79a1 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 1d9979d..bd37a73 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
More information about the bind10-changes
mailing list