BIND 10 #2786: DHCPv4: domain-name option must be encoded as text and NOT as FQDN

[BIND 10 Development]

#2786: DHCPv4: domain-name option must be encoded as text and NOT as FQDN
-------------------------------------+-------------------------------------
            Reporter:  marcin        |                        Owner:
                Type:  defect        |  stephen
            Priority:  high          |                       Status:
           Component:  dhcp4         |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20130523
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:
                                     |  Medium
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by marcin):

 * owner:  marcin => stephen


Comment:

 Replying to [comment:6 stephen]:
 > Reviewed commit commit b3c562f2bd85fa9f825d59f8601ba99e04e661f3
 >
 > '''src/lib/dhcp/option_string.{cc,h}'''[[BR]]
 > General: the implementation stores the string in a class member
 variable, which duplicates the storage of the base class.  As the "string"
 nature of the class only appears in the {get,set}Value() methods and the
 string constructor, this seems wasteful.  Can't the data be stored in the
 underlying class and converted to/from a string as needed? (As an aside,
 it also means that most process can be done by the parent class methods.)
 Note that this is a suggestion: the amount of code saved is minimal.

 I have modified the class as suggested.

 >
 > setValue/constructor taking a string argument: should check for an empty
 string and throw an exception if this is the case. (This puts the
 exception closer to the point of the error.)

 Done.

 >
 > Exceptions: suggest that the exception message includes the type code of
 the option - it will make diagnosis of any problem easier.
 >
 Added.

 >
 > '''src/lib/dhcp/tests/libdhcp++_unittest.cc'''[[BR]]
 > unpackOptions4 test: trivial point: "v4Opts+n" should have spaces around
 the "+". (This appears in several places in this test)

 I added spaces where applicable. Also, !''v4Opts!'' is the invalid name
 for variable. I renamed it to !''v4_opts!''.

 >
 > stdOptionDefs4 test: interesting correction.  I note that before the
 change, "end" was equal to "begin", yet apparently the test passed.  This
 suggests that testStdOptionDefs4() will always register a "pass"
 regardless of the value of the "end" argument, so either:
 > * "end" is not really needed as an argument.
 > * testStdOptionDefs4() registers a "pass" regardless of its arguments.
 > It's worth investigating this.

 The !''!OptionCustom!'' can be created from the buffer (when decoding
 received packet) or it can be created for the outgoing packet. In the
 former case, it is desired that the constructor performs validation on the
 provided buffer to verify that data is consistent, the buffer is not
 truncated etc. In the latter case, you may want to first create an object
 and later set the data, e.g. using class modifiers.

 !OptionCustom class assumed that if the buffer provided in the constructor
 was empty, the data was supposed to be later set using modifiers and the
 validation of the buffer was not performed on the constructor level (the
 latter use case when sending option in an outgoing packet). Thus, it was
 possible to specify an empty buffer when creating some of the options and
 pass (successfuly create option instance). This is a little odd and
 inconsistent with other classes so I modified !OptionCustom so as it
 always validates the provided buffer if it is provided (even when empty).
 There is one more constructor which doesn't take a buffer argument. If
 this constructor is used, the instance of the !OptionCustom object is
 created and the '''default''' data is set for all option fields. This data
 can be later modified using modifier functions.

 With these changes, the !''libdhcp++_unittest!'' will fail if the provided
 buffer is empty (two the same iterators).

 >
 >
 > '''src/lib/dhcp/tests/option_string_unittest.cc'''[[BR]]
 > Should include a test for setValue() with an empty string.
 >
 > Should include a test for trying to construct a string option using a
 zero-length string.

 As a consequence of the changes to setValue, this is necessary. Added.

 >
 >
 > '''src/lib/dhcp/tests/pkt4_unittest.cc'''[[BR]]
 > unpackOptions test: comment under the "x = pkt->getOption(14)" statement
 is incorrect - it is option 14 that should exist, not option 13. (A
 reference to option 13 also occurs in another comment later in that block
 of code).

 Corrected.

 >
 > '''!ChangeLog entry'''[[BR]]
 > This is OK.

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