BIND 10 #2549: Use IOAddress::fromBytes, isV4 and isV6 functions in DHCP code.

[BIND 10 Development]

#2549: Use IOAddress::fromBytes, isV4 and isV6 functions in DHCP code.
-------------------------------------+-------------------------------------
            Reporter:  marcin        |                        Owner:
                Type:  enhancement   |  stephen
            Priority:  low           |                       Status:
           Component:  dhcp          |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20130103
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by marcin):

 * owner:  marcin => stephen


Comment:

 Replying to [comment:5 stephen]:
 > Reviewed commit 7cdae45e8e18a190a5d2097655f33c0684ef9b9f
 >
 > '''src/lib/dhcp/iface_mgr.cc'''[[BR]]
 > Line 215 & 251: Strikes me that the comment should be "Skip all but V4
 addresses" with the test as "if (!addr->isV4())" and "Skip V6 addresses"
 if the test is "if (addr->isV6())".
 Changed.

 >
 > openSocket6(): a memcpy is dones from "addr", but there is no check that
 the address passed is a V6 address.  This means that the memcpy() coule be
 copying from beyond the end of a four-byte array.
 Note that the openSocketX() functions are called from openSocket() only.
 In this function there is a check for the address type and the
 openSocketX() is called accordingly. In this case there is no way that the
 IPv4 address is passed to this function with the current code. Also, this
 function is not in a public scope.

 >
 > Line 953: comment is wrong - are only dealing with IPv6 addresses.

 Corrected.

 >
 > '''src/lib/dhcp/option_definition.cc'''[[BR]]
 > Line 385: as the address is neither V4 nor V6, the test in the exception
 message will always cause "v6" to be printed.  A message saying that
 "provided address <text> is not a valid V4 or V6 address" is required.

 Good catch. Corrected.

 >
 > ''''src/lib/dhcpsrv/addr_utilities.cc'''[[BR]]
 > {first,last}AddrInPrefix6: "packed" is declared as length V6ADDRESS_LEN,
 then 16 bytes is copied into it.  The "memcpy" should use V6ADDRESS_LEN as
 the length of the data to copy.  Or the code should BOOST_STATIC_ASSERT
 that 16 <= V6ADDRESS_LEN.

 I use the constant (instead of 16) now.

 >
 > '''src/lib/dhcpsrv/alloc_engine.cc'''[[BR]]
 > increaseAddress: As "packed" is used for both V4 and V6 addresses, the
 code should include a BOOST_STATIC_ASSERT to check that V6ADDRESS_LEN is
 greater than or equal to V4ADDRESS_LEN (thus ensuring that the buffer is
 large enough for both types of addresses).

 BOOST_STATIC_ASSERT is a nice trick I never used before. I used it here as
 suggested.


 >
 > increaseAddress: the only reason for the "isV4()" check is to determine
 what amount of data is being returned.  But that can be done by using the
 intermediate vector returned by toBytes() and using its size() call to
 determine the amount of data to be copied.  A check should be made that
 the size of the data returned is less than or equal to the size of the
 "packed" buffer.

 You're right. That way the code is shorter.

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