BIND 10 #703: Testing resolver with bad packets

[BIND 10 Development]

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

 * owner:  stephen => vorner
 * estimatedhours:  3.0 => 8


Comment:

 > 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.
 Mainly because I'm more familiar with C++ and the various C++ libraries we
 use to handle sending and receiving DNS messages, so it was faster to
 write it that language.  Also, since the program uses the existing IOFetch
 class for the I/O, the code essentially consists of little more than
 parsing options and setting fields in a message buffer.

 > 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).
 I wanted to wrap this ticket up why I restricted it to just setting flags.
 However in the changes made to this review I have extended it to include
 setting the various section counts and to artificially alter the message
 length.  The README file (and a TODO comment in badpacket.cc) contain
 these suggestions and I will create additional tickets for the extension.

 > Did you run it against the server? Everything OK?
 Duh!  I was so focused on getting the code into a state where it could be
 included in the distribution, I omitted to report on the tests. I've run
 the current version of the program against the current development version
 of the resolver - the results are in the
 [attachment:BadPacketTests_2011-04-13.txt attachment] to this ticket.  In
 summary, the resolver passed the tests, although I do have a question as
 to the rcode returned when a NOTIFY message is received.

 > We now have tests/tools and tools folders. Do we want to unify it
 somehow?
 That's a longer-term goal.  The idea behind this program is to make it one
 of the system tests (which is why it is in the test folder) rather than to
 make it a fully-supported utility.  Hence placing it in tests.


 > 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? ...
 The reason was that the tool is not intended for general use, so I assumed
 that the person using it would know what the bounds for the flags are.

 However, you are right - since the code checks the value given against
 limits, there is no reason why it can't output an error message if the
 value given falls outside these limits.  The code has been changed to
 report the error.


 > 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.
 That's what you get when you focus on a limited set of objectives and not
 on extensibility.  I have altered it to use arrays and made the changes
 you suggested.  And in fairness, it did make it easier to make the
 extensions I described above.

 > BTW, don't we have such constants as these somewhere in the code?
 We have some, but not all, in src/lib/dns/message.h (e.g. we don't have
 the offsets).  For completeness I specified them all here.

 > There are tests for the code, so I guess this is something we want to
 distribute as a tool for people to use.
 No - all code should be tested, even if we aren't distributing it.
 Whether we want this as a fully-supported tool is something that needs to
 be discussed.

 > ...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?
 The .cc files for which there are no tests are badpacket.cc - which
 contains little more than "parse command line then call the scan" and
 scan.c, the scan itself.  The latter is difficult to test as it actually
 sends and receives packets.  However, a visual check of the output
 confirms that the scan module is behaving as expected.

 > TODO: Need to randomise the source port
 This was left over from an earlier implementation of IOFetch.  As port
 randomisation has been included in a previous ticket, the comment has been
 removed.

 > You mention that port has a default of 53, but the function header has
 no default at the port argument.
 This was missed in a previous ticket.  The comment has been removed as the
 "port" argument is followed by arguments that have no defaults.

 > You have a different ordering of '\param' directives than the real
 parameters. This could be misleading.
 This was missed in a previous ticket.  Fixed.

 > Typo/missing word? „that invalid in a query“.
 Fixed.

 > This is missing something...
 Fixed.

 > Did we talk about getopt_long? I don't really remember, but is it
 available everywhere?
 The last time I looked I it was, although on one system the calling
 sequence is different (there is an additional argument).  I thought that
 this was Solaris but that's not the case - the code built on Solaris, OS X
 and FreeBSD.  In this case getopt_long is needed because there are so many
 cryptic options, so if the build fails I'll add an #ifdef for that
 operating system.

 > You talk about boost::program_options namespace, but I didn't find
 anything using it.
 Removed.  Originally I was going to use program_options but this requires
 linking in another library.

 > This isn't very helpful (I can parse it, but some friendlier error
 wouldn't hurt):
 The bad lexical cast exception is now intercepted and a more friendly
 message is output.

 Thank you for the helpful review.

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