BIND 10 #703: Testing resolver with bad packets

[BIND 10 Development]

#703: Testing resolver with bad packets
-------------------------------------+-------------------------------------
                 Reporter:  stephen  |                Owner:  stephen
                     Type:           |               Status:  reviewing
  enhancement                        |            Milestone:
                 Priority:  major    |  Sprint-20110419
                Component:           |           Resolution:
  Unclassified                       |            Sensitive:  0
                 Keywords:           |  Add Hours to Ticket:  0
Estimated Number of Hours:  3.0      |          Total Hours:  0
                Billable?:  1        |
                Internal?:  0        |
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => stephen


Comment:

 Hello

 First, something that probably will not result in any code changes, but:
  * Why `C++`? Wouldn't python be enough? I believe the performance here is
 not that important (after all, single run can generate some 64k packets
 only). Not that it would make sense to rewrite it now, of course.
  * When this is merged, is it expected that the ticket will continue?
 Because this, for sure, aren't all possible ways to break the packet, I'd
 guess things like setting a name compression pointer or some length to get
 out of the packet would be more dangerous (I could imagine the server to
 crash more willingly to such broken packet than just a strange bits, in
 which case I would expect a broken server to simply misbehave).
  * Did you run it against the server? Everything OK?
  * We now have `tests/tools` and `tools` folders. Do we want to unify it
 somehow?

 Then, some high-level observations:
  * You seem to use the DWIM behaviour in case of ranges, etc. Isn't it
 better to complain loudly if I set a range of flags 1-10000? Because then,
 I'm surprised it won't send 1000 packets at the server. Or I could have
 just mistyped it and didn't notice, so I spent quite some time observing
 the wrong results. Also, you do it not only with the parser, but with
 setFlag methods ‒ if any code tries to put something too large there, the
 code contains a bug and it should fail I believe. Or is there any reason
 for this behaviour I didn't notice?
  * You name each flag as separate symbol, function, etc. That leads to
 quite repetitive long code, really nested code, etc. If there was an enum
 of the flags, array of the masks and shifts for each of them, and the
 flags was an array as well, a lot of the code could be made a lot shorter.
 The only disadvantage would be it would be called something like this:
  {{{#!cpp
 flags.setFlag(RCODE, 3);
  }}}

  which can be little bit more error prone (eg. there's a need to check
 that the passed enum is in range).

  BTW, don't we have such constants as these somewhere in the code?
  * There are tests for the code, so I guess this is something we want to
 distribute as a tool for people to use. I'm not sure if everything is
 tested, there's quite some code, but it seems there are less test filest
 than the .cc files. Or did I miscount?

 Minor points about the actual code:
  *
 {{{#!cpp
 /// TODO: Need to randomise the source port
 }}}

    You have this in the middle of doxygen. Is it really something to be in
 the API level documentation, shouldn't the TODO be inside the
 implementation? And if so, wouldn't `\todo` be better, since doxygen
 understands it and creates indices of all such todos in the code, not to
 mention nicer formatting.

  * You mention that port has a default of 53, but the function header has
 no default at the port argument.
  * You have a different ordering of '\param' directives than the real
 parameters. This could be misleading.
  * Typo/missing word? „that invalid in a query“.
  * This is missing something:
  {{{#!cpp

  /// Parses the command-line options and returns the results in an Options
  /// structure.  If the
  void
  }}}
  * Did we talk about `getopt_long`? I don't really remember, but is it
 available everywhere?
  * You talk about `boost::program_options` namespace, but I didn't find
 anything using it.
  * This isn't very helpful (I can parse it, but some friendlier error
 wouldn't hurt):
  {{{
  19:03 vorner at hydra ~/work/bind10/tests/tools/badpacket $ ./badpacket --qr
 bla
  terminate called after throwing an instance of
 'boost::exception_detail::clone_impl<boost::exception_detail::error_info_injector<boost::bad_lexical_cast>
 >'
    what():  bad lexical cast: source type value could not be interpreted
 as target
    Aborted (core dumped)
  }}}

 With regards

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