BIND 10 #2749: kill io_utilities.h or make it safe
BIND 10 Development
do-not-reply at isc.org
Fri Jan 24 18:59:28 UTC 2014
#2749: kill io_utilities.h or make it safe
-------------------------------------+-------------------------------------
Reporter: jinmei | Owner: muks
Type: defect | Status:
Priority: medium | reviewing
Component: Unclassified | Milestone:
Keywords: | Sprint-20131015
Sensitive: 0 | Resolution:
Sub-Project: Core | CVSS Scoring:
Estimated Difficulty: 3 | Defect Severity: N/A
Total Hours: 0 | Feature Depending on Ticket:
| Add Hours to Ticket: 0
| Internal?: 0
-------------------------------------+-------------------------------------
Changes (by stephen):
* owner: stephen => muks
Comment:
Reviews commit c7aeb4bec30cf6373e77216058174d6d35850dde
'''src/bin/dhcp6/dhcp6_srv.cc'''[[BR]]
generateServerID(): the construct "srvid.size() - n" is used to ensure
that the buffer is large enough for the writeUintNN() calls. However, as
the preceding line declares the !OptionBuffer with a size of at least 8, I
think this construct - although general - is unnecessary. I suggest
preceding the three write statements with a comment along the lines of "We
know that srvid is at least 8 bytes in length, so...". This makes the
code marginally faster.
(Note that the assumption that there is enough space in a buffer is made
in unpackOptions(), where an explicit length is passed to the readUint16()
calls. In that case, the assumption is made in the "while" condition in
that the readUint16() functions won't be executed if there is less than
four bytes remaining.)
'''src/lib/asiodns/tests/io_fetch_unittest.cc'''[[BR]]
Although "sizeof" is a keyword, the general usage elsewhere in BIND 10
appears to be treat it as a function. So the "standard" appears to write
it as "sizeof(xxx)". not "sizeof (xxxgdif )". The embedded space should
be removed.
'''src/lib/asiolink/tests/tcp_socket_unittest.cc'''[[BR]]
See comment for io_fetch_unittest.cc
!SequenceTest: to be really generic, TCPCallback::MIN_SIZE could be used
as the size argument for writeUint16(), as that is the size to which the
data buffer returned by server_cb.data() is set.
'''src/lib/asiolink/tests/udp_socket_unittest.cc'''[[BR]]
See comment for io_fetch_unittest.cc
'''src/lib/dhcp/option.cc'''[[BR]]
As readUintNN() now checks the size and throws the !OutOfRange exception
if the size is invalid, the equivalent checks in Option::getUintNN() can
be removed.
Although not strictly part of this change, in the setUintNN() functions
the argument to the data_.resize() function call could be a
"sizeof(datatype)" instead of an explicit literal.
'''src/lib/dhcp/option4_addrlst.cc'''[[BR]]
'''src/lib/dhcp/option6_ia.cc'''[[BR]]
'''src/lib/dhcp/option6_iaaddr.cc'''[[BR]]
'''src/lib/dhcp/option6_iaprefix.cc'''[[BR]]
'''src/lib/dhcp/option_int.h'''[[BR]]
'''src/lib/dhcp/option_int_array.h'''[[BR]]
'''src/lib/dhcp/option_vendor.cc'''[[BR]]
I'm not sure that use of "distance" is strictly correct here. The
readUintNN() calls require as argument the number of bytes in the buffer,
but "distance" returns the number of elements (which may not be equal to
the number of bytes) between two pointers. Use of "distance" relies on
!OptionBufferConstIterator being a pointer to a byte. However, as most of
the code here in these files make that (valid) assumption, I don't think
it is worth changing.
'''src/lib/resolve/tests/recursive_query_unittest_2.cc'''[[BR]]
'''src/lib/resolve/tests/recursive_query_unittest_3.cc'''[[BR]]
'''src/lib/util/io_utilities.h'''[[BR]]
'''src/lib/util/tests/io_utilities_unittest.cc'''[[BR]]
See comment for io_fetch_unittest.cc
--
Ticket URL: <http://bind10.isc.org/ticket/2749#comment:14>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list