BIND 10 #2905: buggy zone should result in SERVFAIL, not REFUSED

[BIND 10 Development]

#2905: buggy zone should result in SERVFAIL, not REFUSED
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  defect        |  jinmei
            Priority:  medium        |                       Status:
           Component:  data source   |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130528
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  5             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => jinmei


Comment:

 Hello

 I know this should not cause any disruption, but isn't it better to keep
 the values of constants and allocate new numbers for the new ones?
 {{{#!diff
 +    static const ZoneNode::Flags EMPTY_ZONE = ZoneNode::FLAG_USER2;
  public:
      /// \brief Node flag indicating it is at a "wildcard level"
      ///
      /// This means one of the node's immediate children is a wildcard.
 -    static const ZoneNode::Flags WILDCARD_NODE = ZoneNode::FLAG_USER2;
 +    static const ZoneNode::Flags WILDCARD_NODE = ZoneNode::FLAG_USER3;
 }}}

 I think the `FindeResult::flags` in the zone table might benefit from
 little documentation.

 The exception text „Zone content must not be NULL or empty“ is
 understandable, but only with some thinking about what it means. Should it
 better be „nor“?

 The comment about empty at the first line here doesn't hold any more (or,
 is confusing at least, as empty now means something else than NULL).
 {{{#!diff
 The empty here no longer applies
      // It doesn't accept empty (NULL) zones
      EXPECT_THROW(zone_table->addZone(mem_sgmt_, zclass_, zname1, NULL),
 -                 isc::BadValue);
 +                 isc::InvalidParameter);
      EXPECT_EQ(0, zone_table->getZoneCount()); // count is still 0

 +    // or an empty zone
 +    SegmentObjectHolder<ZoneData, RRClass> holder_empty(
 +        mem_sgmt_, zclass_);
 }}}

 Would it be better to reset the header after the whole try-catch block?
 {{{#!c++
 void
 ZoneWriter::load(std::string* error_msg) {
     if (state_ != ZW_UNUSED) {
         isc_throw(isc::InvalidOperation, "Trying to load twice");
     }

     try {
         zone_data_ = load_action_(segment_.getMemorySegment());
         segment_.resetHeader();

         if (!zone_data_) {
             // Bug inside load_action_.
             isc_throw(isc::InvalidOperation,
                       "No data returned from load action");
         }
     } catch (const ZoneLoaderException& ex) {
         if (!catch_load_error_) {
             throw;
         }
         if (error_msg) {
             *error_msg = ex.what();
         }
         segment_.resetHeader();
     }

     state_ = ZW_LOADED;
 }
 }}}

 This doxygen example could be more consistent in calling methods instead
 of having comments to do something. So, instead of „broken zone, handle it
 accordingly“, should it be something like `createServFail();`?
 {{{#!c++
     ///   if (result.datasrc_) {
     ///       if (result.finder_) {
     ///           createTheAnswer(result.finder_);
     ///       } else {
     ///           // broken zone, handle it accordingly.
     ///       }
     ///   } else {
     ///       createNotAuthAnswer();
     /// } \endcode
 }}}

 Should this check the result is actually empty (eg. has the flag
 somewhere)?
 {{{#!c++
     // check the result with empty (broken) zones.  Right now this can
 only
     // happen for in-memory caches.
     void emptyResult(const ClientList::FindResult& result, bool exact,
                      const char* trace_txt)
     {
         SCOPED_TRACE(trace_txt);
         ASSERT_FALSE(result.finder_);
         EXPECT_EQ(exact, result.exact_match_);
         EXPECT_TRUE(dynamic_cast<InMemoryClient*>(result.dsrc_client_));
     }
 }}}

 The `loadLoaderException` test should probably test passing NULL error
 parameter, to check it does not crash by assigning to the pointer
 unconditionally.

 Just to make sure, the changes in
 `2f0e203d44adda18249239ff33a75102a8477e82` are not yet used at all in that
 commit and are preparation for the later commits, are they?

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