BIND 10 #2765: b10-dhcp4 silently fails if bootp/dhcp4 port is already used on one interface
BIND 10 Development
do-not-reply at isc.org
Tue Dec 3 13:40:14 UTC 2013
#2765: b10-dhcp4 silently fails if bootp/dhcp4 port is already used on one
interface
-------------------------------------+-------------------------------------
Reporter: cas | Owner: tmark
Type: defect | Status:
Priority: medium | reviewing
Component: dhcp4 | Milestone:
Keywords: | Sprint-DHCP-20131204
Sensitive: 0 | Resolution:
Sub-Project: DHCP | CVSS Scoring:
Estimated Difficulty: 0 | Defect Severity: N/A
Total Hours: 0 | Feature Depending on Ticket:
| Add Hours to Ticket: 0
| Internal?: 0
-------------------------------------+-------------------------------------
Changes (by marcin):
* owner: marcin => tmark
Comment:
Replying to [comment:11 tmark]:
> This was tricky bit of work. The use of the error handler is a good
approach.
Thanks.
I apologize but I had one outstanding commit which adds a new paragraph to
the developer's guide. I forgot to push this commit prior to the review
that you have done. I pushed it now.
>
>
-------------------------------------------------------------------------
> pkt_filter.h and pkt_filter.cc
>
> Please check your line width. Quite a few comment lines that spill over
80.
Fixed.
>
>
-------------------------------------------------------------------------
> pkt_filter.h
>
> openFallbackSocket() has no @throw.
Added
>
>
-------------------------------------------------------------------------
> pkt_filter_inet.h -
>
> Commentary for openSocket() has no @throw. Neither does receive(), nor
send().
Added.
One comment: in some cases it is a rat hole to add throw tags for all
types of exceptions being thrown because they very much depend on the
exceptions thrown by functions they call. Even worse, exceptions thrown by
functions being called from the particular function may change and all
dependent throw tags would have to be updated accordingly.
>
>
-------------------------------------------------------------------------
> 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.
I consider it a good idea. So, I added it.
>
>
-------------------------------------------------------------------------
> 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.
>
First, I think you meant this:
{{{
return (SocketInfo(addr, port, sock, fallback));
}}}
Secondly, I don't quite understand your point. In both cases the
SocketInfo structure is created on the stack - they are local variables.
The only difference is that in the latter case, we create an anonymous
variable which may be used in the point where it has been created and
immediately destroyed - doesn't live in the function scope like in the
former case. Before it is destroyed it is copied and returned. Same in the
former case: the variable is created, copied and destroyed. They are even
destroyed in the same time, because the function hits !''return!''
immediately after creation of the named variable.
In addition, both versions produce exactly the same assembler code.
>
-------------------------------------------------------------------------
> 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.
I was thinking that I should return on error, and 0 is not an indication
of error. But yes, there is rather no chance that I will get anymore data
from this socket if it returned 0. So I changed that as requested.
>
> 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?
I was considering this issue. Ultimately, I decided not to implement it,
because I don't know if such a problem even has any chance to exist. If it
does, it can be a problem of any (not only fallback) socket we open. On
the other hand, during the normal operation both primary and fallback
socket should be in sync - they are configured to listen/filter the same
traffic. So, I really don't think it is a problem but I added a todo to
reconsider this design if needed.
>
>
-------------------------------------------------------------------------
> 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.
Fixed.
>
>
-------------------------------------------------------------------------
> 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.
Added quite extensive description.
>
>
-------------------------------------------------------------------------
> iface_mgr_unittest.cc
>
> checkPacketFilterLPFSocket - It would help if this test had some
commentary
> describing what it tests.
>
I added test description.
> 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!" ?
> }}}
I added ADD_FAILURE but it doesn't change much. Note that the following
part of the test:
{{{
// The socket is open and bound. Another attempt to open socket and
// bind to the same address and port should result in an exception.
EXPECT_THROW(
socket2 = iface_mgr2->openSocket(LOOPBACK, loAddr,
DHCP4_SERVER_PORT + 10000),
isc::dhcp::SocketConfigError
);
}}}
already causes the test to fail if the socket opens. The other part is
simply to perform a cleanup after the test if it opens the socket for some
reason.
>
>
-------------------------------------------------------------------------
> 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.
You deserve to take position in the QA team. :) Seriously, thanks for
that. I fixed it - the !IfaceMgr had a bug which caused the fallback
socket to be closed if its descriptor was non-zero (including negative
value).
>
-------------------------------------------------------------------------
>
> The code passed cppcheck, and the unit tests were run under both OS-X
and Debian6.
>
>
You really do deserve being in QA team ;)
--
Ticket URL: <http://bind10.isc.org/ticket/2765#comment:12>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list