BIND 10 #2765: b10-dhcp4 silently fails if bootp/dhcp4 port is already used on one interface

[BIND 10 Development]

#2765: b10-dhcp4 silently fails if bootp/dhcp4 port is already used on one
interface
-------------------------------------+-------------------------------------
            Reporter:  cas           |                        Owner:
                Type:  defect        |  marcin
            Priority:  medium        |                       Status:
           Component:  dhcp4         |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20131204
         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 tmark):

 * owner:  tmark => marcin


Comment:

 This was tricky bit of work.  The use of the error handler is a good
 approach.

 -------------------------------------------------------------------------
 pkt_filter.h and pkt_filter.cc

 Please check your line width. Quite a few comment lines that spill over
 80.

 -------------------------------------------------------------------------
 pkt_filter.h

 openFallbackSocket() has no @throw.

 -------------------------------------------------------------------------
 pkt_filter_inet.h -

 Commentary for openSocket() has no @throw.  Neither does receive(), nor
 send().

 -------------------------------------------------------------------------
 pkt_filter.cc

 openFallbackSocket()

 You may wish to consider adding use of strerror(errno) as was done
 recently in
 iface_mgr.cc.  As it stands, if there are errors there is no OS level
 reason
 for the failure.

 -------------------------------------------------------------------------
 pkt_filter_lpf.cc

 In openSocket(), Why do this?

 {{{
     SocketInfo sock_desc(addr, port, sock, fallback);
     return (sock_desc);
 }}}

 instead of:

 {{{
     return (SocketInfo sock_desc(addr, port, sock, fallback));
 }}}

 Doing it your way, creates a copy locally only to then copies it onto the
 stack
 to return it; instead of making it directly on the stack?  It is also a
 bit
 clearer in my mind, that you intend to return a copy on the stack if you
 use
 the second form.

 -------------------------------------------------------------------------
 pkt_filter_lpf.cc

 In receive(), you loop through reading the fallback socket:

 {{{
     do {
         datalen = recv(socket_info.fallbackfd_, raw_buf, sizeof(raw_buf),
 0);
     } while (datalen >= 0);
 }}}

 First, I think it should be > not >=.

 {{{
 Upon successful completion, recv() shall return the length of the message
 in bytes. If no messages are available to be received and the peer has
 performed an orderly shutdown, recv() shall return 0. Otherwise, -1 shall
 be returned and errno set to indicate the error.
 }}}
 You should only try it again if it returns greater than zero.

 Also, is there any chance we could get stuck here if something were
 spewing
 data at that socket? Should we consider some sort of throttle mechanism?

 -------------------------------------------------------------------------
 iface_mgr.h

 handleSocketConfigError
 The brief description says "during operation on the socket".  It is not
 clear,
 from the commentary that this handler is only for certain socket
 operations.
 I think it would be good to list the conditions under which it will be
 invoked.
 Certainly, it should state that it is not called during read, write, or
 close
 operations.

 -------------------------------------------------------------------------
 iface_mgr.h

 openSockets4

 The parameter commentary for error_handler states that "the function will
 continue when the callback returns control.".  It does not say what it
 will
 continue doing.  There is no detailed discussion for what the function
 does.
 You should add a descriptive paragraph for the function.  It should
 perhaps
 state the ramifications of the caller's handler doing something like
 throwing.

 -------------------------------------------------------------------------
 iface_mgr_unittest.cc

 checkPacketFilterLPFSocket - It would help if this test had some
 commentary
 describing what it tests.

 AT 1137, you have this:

 {{{
     // Surprisingly we managed to open another socket. We have to close it
     // to prevent resource leak.
     if (socket2 >= 0) {
         close(socket2);
     }
 }}}

 Shouldn't this be a test failure?  The whole intent is to not be able to
 open
 a second socket, shouldn't you do something like:
 {{{
  ADD_FAILURE() << "we opened two!" ?
 }}}

 -------------------------------------------------------------------------
 While not serious, running the unit the disabled tests as root with
 valgrind,
 it warns numerous times about attempting to close invalid file descriptor.
 Here's a snippet:

 {{{
 ==17891== Warning: invalid file descriptor -1 in syscall close()
 [       OK ] IfaceMgrTest.socket4 (5 ms)
 [ RUN      ] IfaceMgrTest.openSockets4
 ==17891== Warning: invalid file descriptor -1 in syscall close()
 ==17891== Warning: invalid file descriptor -1 in syscall close()
 [       OK ] IfaceMgrTest.openSockets4 (13 ms)
 [ RUN      ] IfaceMgrTest.openSockets4IfaceDown
 ==17891== Warning: invalid file descriptor -1 in syscall close()
 [       OK ] IfaceMgrTest.openSockets4IfaceDown (8 ms)
 [ RUN      ] IfaceMgrTest.openSockets4IfaceInactive
 ==17891== Warning: invalid file descriptor -1 in syscall close()
 [       OK ] IfaceMgrTest.openSockets4IfaceInactive (9 ms)
 [ RUN      ] IfaceMgrTest.openSockets4NoErrorHandler
 ==17891== Warning: invalid file descriptor -1 in syscall close()
 ==17891== Warning: invalid file descriptor -1 in syscall close()
 [       OK ] IfaceMgrTest.openSockets4NoErrorHandler (10 ms)
 [ RUN      ] IfaceMgrTest.openSocket4ErrorHandler
 ==17891== Warning: invalid file descriptor -1 in syscall close()
 ==17891== Warning: invalid file descriptor -1 in syscall close()
 [       OK ] IfaceMgrTest.openSocket4ErrorHandler (9 ms)
 }}}

 You should probably fix this.
 -------------------------------------------------------------------------

 The code passed cppcheck, and the unit tests were run under both OS-X and
 Debian6.

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