BIND 10 #2836: update in-memory zone loading so it can work with shmem segment

[BIND 10 Development]

#2836: update in-memory zone loading so it can work with shmem segment
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  jinmei
            Priority:  medium        |                       Status:
           Component:  data source   |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130528
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  4             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |  shared memory data source
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:20 vorner]:

 > > > I also pushed the test that crashes for me. Does it crash for you
 too? Does valgrind work for you? If so, could you run it through it?
 > >
 > > That test crashed in my environment, too (though in a different place
 > > of the code).  I've found one issue and committed a fix to that (my)
 > > problem at 575854a507052ad7de51aa259fd5d8f04156d15d on branch
 > > trac2836-2.  Hopefully it will fix yours, too.  Note that we may or
 > > may not use the 5758... fix as it is (see below).
 >
 > I incorporated changes inspired by yours, and the separate set as well.
 The test now passes, so I hope it works as it should. But I'm still not
 100% sure there's no other bug like this.

 There still seem to be a few subtle cases as pointed out below, but
 I'm now getting quite sure it's much safer thanks to the revised
 version of `SegmentObjectHolder`.

 > > - depending on how we revise `SegmentObjectHolder`, `result.zone_data`
 > >   could also be relocated on return of `addZone`.
 >
 > I don't see how that could happen or how to protect against that. Can
 you explain where it would relocate?

 It now shouldn't happen.  Previously the construction of
 `SegmentObjectHolder` could have caused relocation, and `addZone()`
 could still return successfully.  Now it doesn't happen as the
 constructor throws in that case.  See also below.

 > IMO this should be all the functionality for the branch. Or I hope so.
 Many thanks for pointing me to the problem of the test.

 It now looks almost ready.  I have a few less critical points to
 adjust (mostly for documentation and cleanups):

 '''others'''
 - I made a few minor changes as usual.
 - Among them is a build fix regarding `USE_SHARED_MEMORY` (d35ac0a).
   This also suggest you might want to check if the branch can build
   and pass tests `--without-shared-memory` (once again, even if you've
   done that once)
 - I suggest clarifying exception/relocation safety in various
   create() methods.  For example, for `ZoneData::create()`,
 {{{#!cpp
     /// \brief Allocate and construct \c ZoneData.
     ///
     /// This method ensures there'll be no memory leak on exception.
     /// But 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.  On successful return, this method ensures there's no
     /// address relocation.
     ///
     /// \throw util::MemorySegmentGrown memory segment has grown, possibly
     /// causing address relocation.
     /// \throw std::bad_alloc Memory allocation fails.
 }}}
   I believe this generally applies to `NSEC3Data::create()`,
   `ZoneTable::create()`, `ZoneTable::addZone()` and `RdataSet::create()`.

 '''segment_object_holder.h'''
 - I'm afraid the relationship between the constructor and set is not
   obvious and requires explanation.  I'd suggest adding the following
   note as a comment:
 {{{#!cpp
 // more simplified.
 //
 // Note, however, that it doesn't take the pointer to hold on
 construction.
 // This is because the constructor itself can throw or cause address
 // reallocation inside the memory segment.  If that happens various
 // undesirable effects can happen, such as memory leak or unintentional
 access
 // to the pre-reallocated address.  To make it safer, we use a separate
 // \c set() method, which is exception free and doesn't cause address
 // reallocation.  So the typical usage is to first construct the holder
 // object, immediately followed by a call to \c set().  Subsequent access
 // to the held address should be done via the \c get() method.  get()
 ensures
 // the address is always valid in the memory segment even if address
 // reallocation happens between set() and get().
 //
 // template parameter T is the type of object allocated by mem_sgmt.
 }}}

 '''zone_data_updater.cc'''

 - We shouldn't need this hack anymore thanks to the revised
   `SegmentObjectHolder`:
 {{{#!cpp
         // The create above might have relocated data. So get it again,
         // just to make sure.
         zone_data_ =
             static_cast<ZoneData*>(mem_sgmt_.
                                    getNamedAddress("updater_zone_data").
                                    second);
 }}}
   (see also the suggested doc update for xxx:create() above).
   Previously the construction of `SegmentObjectHolder` itself could
   cause relocation, which was the reason for the crash and the need
   for this hack.  Now it shouldn't happen (instead `MemorySegmentGrown`
   will be thrown, which will be handled at an appropriate upper
   layer).

 '''zone_data_updater.h'''

 - there's a slight chance that `zone_data_` is relocated here:
 {{{#!cpp
         mem_sgmt_.setNamedAddress("updater_zone_data", zone_data_);
 }}}
   so we'll need to make sure it's the latest:
 {{{#!cpp
         mem_sgmt_.setNamedAddress("updater_zone_data", zone_data_);
         zone_data_ =
 mem_sgmt_.getNamedAddress("updater_zone_data"_).second;
         assert(zone_data_);
 }}}

 '''zone_writer.cc'''

 - load(): it's probably safer to call `resetHeader` immediately after
   `load_action_`:
 {{{#!cpp
     zone_data_ = load_action_(segment_.getMemorySegment());
     segment_.resetHeader(); // header may have been relocated

     if (!zone_data_) {
         // Bug inside load_action_.
         isc_throw(isc::InvalidOperation, "No data returned from load
 action");
     }
 }}}
 - install: we should move the call to `resetHeader` in the catch
         block:
 {{{#!cpp
         } catch (const isc::util::MemorySegmentGrown&) {
             segment_.resetHeader();
         }
 }}}
   and there's now no need for calling it at the end of the method,
   because addZone() ensures no relocation on successful return
   (see above).

-- 
Ticket URL: <http://bind10.isc.org/ticket/2836#comment:21>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development