[2973] do not destroy passed content due to exception in ZoneTable::addZone.

this will cause use-after-free in ZoneWriter.  Instead, clarify that
the caller is responsible to deal with that unless the method successfully
returns.  An assumption of the ZoneTable.addZone test is now broken due to
that, so it was adjusted.

src/lib/datasrc/memory/zone_table.cc

@@ -83,7 +83,7 @@ ZoneTable::destroy(util::MemorySegment& mem_sgmt, ZoneTable* ztable, int)
 }
 
 ZoneTable::AddResult
-ZoneTable::addZone(util::MemorySegment& mem_sgmt, RRClass zone_class,
+ZoneTable::addZone(util::MemorySegment& mem_sgmt, RRClass,
                    const Name& zone_name, ZoneData* content)
 {
     LOG_DEBUG(logger, DBG_TRACE_BASIC, DATASRC_MEMORY_MEM_ADD_ZONE).
@@ -94,13 +94,8 @@ ZoneTable::addZone(util::MemorySegment& mem_sgmt, RRClass zone_class,
                   (content ? "empty data" : "NULL") <<
                   " is passed to Zone::addZone");
     }
-    SegmentObjectHolder<ZoneData, RRClass> holder(mem_sgmt, zone_class);
-    holder.set(content);
 
-    const AddResult result =
+    return (addZoneInternal(mem_sgmt, zone_name, content));
-        addZoneInternal(mem_sgmt, zone_name, holder.get());
-    holder.release();
-    return (result);
 }
 
 ZoneTable::AddResult

src/lib/datasrc/memory/zone_table.h

@@ -165,12 +165,22 @@ public:
     ///
     /// This method adds a given zone data to the internal table.
     ///
-    /// This method ensures there'll be no memory leak on exception.
+    /// On successful completion (i.e., the method returns without an
-    /// But addresses allocated from \c mem_sgmt could be relocated if
+    /// exception), the ownership of \c content will be transferred to
-    /// \c util::MemorySegmentGrown is thrown; the caller or its upper layer
+    /// the \c ZoneTable: the caller should not use the \c content hereafter;
-    /// must be aware of that possibility and update any such addresses
+    /// the \c ZoneTable will be responsible to destroy it when the table
-    /// accordingly.  On successful return, this method ensures there's no
+    /// itself is destroyed.
-    /// address relocation.
+    ///
+    /// If this method throws, the caller is responsible to take care of
+    /// the passed \c content, whether to destroy it or use for different
+    /// purposes.  Note that addresses allocated from \c mem_sgmt could be
+    /// relocated if \c util::MemorySegmentGrown is thrown; the caller or its
+    /// upper layer must be aware of that possibility and update any such
+    /// addresses accordingly.  This applies to \c content, as it's expected
+    /// to be created using \c mem_sgmt.
+    ///
+    /// On successful return, this method ensures there's no address
+    /// relocation.
     ///
     /// \throw InvalidParameter content is NULL or empty.
     /// \throw util::MemorySegmentGrown The memory segment has grown, possibly

src/lib/datasrc/memory/zone_writer.cc

@@ -134,9 +134,6 @@ ZoneWriter::install() {
     // zone data or we've allowed load error to create an empty zone.
     assert(impl_->data_holder_.get() || impl_->catch_load_error_);
 
-    // FIXME: This retry is currently untested, as there seems to be no
-    // reasonable way to create a zone table segment with non-local memory
-    // segment. Once there is, we should provide the test.
     while (impl_->state_ != Impl::ZW_INSTALLED) {
         try {
             ZoneTable* table(impl_->segment_.getHeader().getTable());

src/lib/datasrc/tests/memory/zone_table_unittest.cc

@@ -141,14 +141,13 @@ TEST_F(ZoneTableTest, addZone) {
     EXPECT_EQ(3, zone_table->getZoneCount());
 
     // Have the memory segment throw an exception in extending the internal
-    // tree.  It still shouldn't cause memory leak (which would be detected
+    // tree.  We'll destroy it after that via SegmentObjectHolder.
-    // in TearDown()).
     SegmentObjectHolder<ZoneData, RRClass> holder6(
         mem_sgmt_, zclass_);
     holder6.set(ZoneData::create(mem_sgmt_, Name("example.org")));
     mem_sgmt_.setThrowCount(1);
     EXPECT_THROW(zone_table->addZone(mem_sgmt_, zclass_, Name("example.org"),
-                                     holder6.release()),
+                                     holder6.get()),
                  std::bad_alloc);
 }