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
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Thanks for the review.

 Replying to [comment:8 vorner]:

 > 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 actually wondered the same thing.  I chose changing the value so we
 can consolidate the private variables (and yet avoid listing them in a
 counter intuitive order, like USER1, USER3, then USER3).  But,
 especially because we are going to write the image in a mapped file,
 it's probably better to preserve binary compatibility as much as
 possible.  So I revised the code that way (with one related cleanup).

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

 Hmm, not sure exactly which part of th code you meant, but I've tried
 to make some clarification in zone_table.h.

 > 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“?

 (Readability aside) in my understanding "not A or B" is the
 grammatically correct form while "not A nor B" isn't.  But in any case
 I've revised it more fundamentally.  Hopefully that one is clearer.

 > 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_);
 > }}}

 Okay, revised.

 > Would it be better to reset the header after the whole try-catch block?

 I expect in #2850 we'll eliminate the need for `resetHeader()`
 (actually this use of `resetHeader()` was not completely exception
 safe regardless where we call it).  So I've kept this part of the code
 at the moment.  I'll check that again once #2850 is merged (or if this
 branch is merged first, at the time of merging #2850).

 > 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
 > }}}

 Okay, updated.

 > 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_));
 >     }
 > }}}

 It checks that by confirming finder_ is NULL and `dsrc_client_` isn't.
 I considered introducing some more explicitly interface that indicates
 the zone is empty/broken, but adding another boolean didn't seem to be
 a good idea because that one and finder_ and dsrc_client_ are related
 and we'll need to be careful not to break the integrity.  Adding a
 method like `isZoneEmpty()` (return true iff both finder_ and
 dsrc_client_ are non NULL) would be a possible idea, but for the user
 like auth query.cc it didn't seem to be very useful anyway.

 At the moment I didn't touch the code regarding this.

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

 It's at least implicitly tested as NULL is the default parameter
 value.  I clarified that point in a comment.  We might test the case
 of passing NULL explicitly too, but for now I've only made the comment
 clarification, preferring conciseness.

 > 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?

 Correct.  The `QueryTest.emptyZone` test in the next commit relies on
 2f0e20.

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