BIND 10 #1627: Error: Server configuration failed: incomplete textual name

[BIND 10 Development]

#1627: Error: Server configuration failed: incomplete textual name
-------------------------------------+-------------------------------------
                   Reporter:  jreed  |                 Owner:  jinmei
                       Type:         |                Status:  reviewing
  defect                             |             Milestone:
                   Priority:         |  Sprint-20120403
  medium                             |            Resolution:
                  Component:  bind-  |             Sensitive:  0
  ctl                                |           Sub-Project:  Core
                   Keywords:         |  Estimated Difficulty:  2
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by muks):

 * owner:  muks => jinmei


Comment:

 Hi jinmei

 Thank you for the detailed review :)

 Replying to [comment:9 jinmei]:
 > As far as I understand it, the real error in the configuration in
 > question is that origin was *unspecified*, not really an invalid form
 > of it (although "unspecified" could be considered a part of invalid
 > form).  So I think a more helpful error is something that specifically
 > points it out, like this patch:
 >
 > {{{#!diff
 >      BOOST_FOREACH(ConstElementPtr zone_config,
 zones_config->listValue()) {
 >          ConstElementPtr origin = zone_config->get("origin");
 > -        if (!origin) {
 > +        const string origin_txt = origin ? origin->stringValue() : "";
 > +        if (origin_txt.empty()) {
 >              isc_throw(AuthConfigError, "Missing zone origin");
 >          }
 > }}}
 >
 > Then what we'd see on bindctl is:
 >
 > {{{
 > > config show Auth/datasources[0]/zones
 > Auth/datasources[0]/zones[0]/origin   ""      string  (modified)
 > Auth/datasources[0]/zones[0]/file     ""      string  (modified)
 > > config commit
 > Error: Missing zone origin
 > Configuration not committed
 > }}}
 > (this could be even more helpful if it said in which configuration it
 > fails (e.g. "Missing zone origin for in-memory zone #0"), but that's
 > not exactly the point of this ticket).
 >
 > Same for file, btw.

 You're right. I've changed both missing checks now. :)

 > Ideally, the spec file should be more descriptive so it can be
 > rejected at that level, but as far as I know we need to provide some
 > default anyway (in this case, an empty string).  So, technically, we
 > cannot distinguish intentionally specified empty string from
 > unspecified config, but in practice if an empty string is passed it's
 > most likely that the user forgets to fill in it.
 >
 > If you agree on this, we could more localize the fixes (we don't have
 > to modify name.h, etc).  But I'll comment on the changes in the branch

 I agree, but please elaborate further.


 > to modify name.h, etc).  But I'll comment on the changes in the branch
 > anyway:
 >
 > - as convention we generally add '[ticket #]' in the beginning of each
 >   commit log message.  So, in this case it should be '[1627]'.  If you
 >   don't have a particular reason not to follow this convention, I'd
 >   suggest you do the same for consistency.

 [number] is parsed by Trac as a link to a commit. #number is parsed by
 Trac
 and linked to a bug. Instead of using #number, I used "bug #number"
 because
 when editing a git commit log in an editor, lines that start with # are
 assumed as comments and not included in the commit log message.

 > - there are some style issues.  I've directly fixed them on the
 >   branch.  Please check it.  In particular, there were some "hard tabs
 >   (\t)", which we generally don't use.

 *nod* :)

 > - clarifying the error message when the given name is invalid would
 >   itself be good, and since this branch isn't big so I'm okay with
 >   doing it here even if we agree the essential issue is not the
 >   validity of the name.  However, I'd like to avoid adding try-catch
 >   for each of this level of things.  If we use exception this way in
 >   the configuration checker, it will be full of such micro try-catch
 >   blocks as the syntax becomes more complicated.  I'd rather keep some
 >   context while we parse the configuration, and just let an exception
 >   thrown from a lower layer (like the dns::Name class) be propagated
 >   at a higher level like configureAuthServer().  We can catch the
 >   exception there, convert it to `AuthConfigError` with the saved
 >   context information:
 > {{{#!c++
 >     } catch (const isc::Exception& ex) {
 >         isc_throw(AuthConfigError, "Server configuration failed at "
 >                   << printContext() << ": " << ex.what());
 >     }
 > }}}
 >   Then the result would look like:
 > {{{
 > Error: Server configuration failed at parsing zone origin of in-memory
 zone #0: <error message from a lower layer>
 > }}}

 I agree on avoiding inner try-catch blocks, but am not sure if maintaining
 a context would be less work than prepending the context and rethrowing,
 when something is thrown.

 >
 > - Regarding the exceptions in the name class, while it might be beyond
 >   the scope of this ticket, one thing we should have done before is to
 >   add the text that triggers the exception, e.g.
 > {{{#!c++
 >                     isc_throw(EmptyLabel, "non terminating empty label
 in " <<
 >                               namestring);
 > }}}
 >

 *nod*. This is implemented now, and in some places checks for an empty
 string too where it can happen.

 > - I think it a good idea to introduce some intermediate class
 >   hierarchy in for exception classes in libdns++ (in fact there should
 >   be a ticket for that, I think), but I'd like to do it based on some
 >   consistent design consideration, rather than by introducing an
 >   intermediate class in ad hoc manner as we see the need for it.
 >   e.g., isc::Exception => isc::dns::Exception => isc::dns::NameError
 >         => isc::dns::NameParserError => isc::dns::EmptyLabel

 This is perhaps good for another ticket too. I had a reason to add the
 NameParserException parent for this bug. :) We can change it however we
 want.

 >
 > - In name_unittest.cc many cases seem to be a result of copy-paste.
 >   I'd like to unify these cases like:
 >
 > {{{#!c++
 > template <typename ExceptionType>
 > void
 > checkBadTextName(const string& txt) {
 >     // Check it results in the specified type of exception as well as
 >     // NameParserException.
 >     EXPECT_THROW(Name(txt, false), ExceptionType);
 >     EXPECT_THROW(Name(txt, false), NameParserException);
 > }
 > ...
 >     checkBadTextName<EmptyLabel>(".a");
 > }}}

 Ouch! It should've been written this way. :) I have modified it to use
 this now. (Also updated a testcase for a different bug to use a helper
 function similarly.)

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