Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#4067 closed defect (fixed)

PktFilterTestStub's use of fd 0 causes issues in unit tests

Reported by: tmark Owned by: tmark
Priority: high Milestone: Kea1.0-beta
Component: dhcp Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

For months we have periodically chased mysterious test failures, where a test will open a resource such as a socket and be assigned fd 0, only to find that fd 0 becomes inexplicably invalid later on. Up til now, it was thought that GTEST was somehow manipulating stdin and causing this issue.

Turns out we've been doing it to ourselves all along. The PktFilterTestStub?, which is used to provide stubbed packet filters for interfaces under test, sets the SocketInfo::sock_fd_ for every filter opened to 0.

Whenever the associated sockets are closed... we end up calling close(0). For large grained tests such as those written for the servers, this happens every time the server is reconfigured or stopped. In fact some of the test fixtures explicitly call IfaeMgr::closeSockets().

Whatever the intent, be it to read test data from stdin or not, the stub needs to obtain an fd by opening a resource such as unix socket, or tmp file, or dupping stdin. That way the fd will be seen as unavailable by any other system call. It cannot use a hard-coded value of 0.

Subtickets

Attachments (1)

toms.patch (917 bytes) - added by tmark 4 years ago.
working patch

Download all attachments as: .zip

Change History (11)

comment:1 Changed 4 years ago by fdupont

The tradition is to open /dev/null when no special property is required.

Changed 4 years ago by tmark

working patch

comment:2 Changed 4 years ago by hschempf

  • Milestone changed from Kea-proposed to Kea1.0

per team meeting sept 23, move to 1.0 high, est 2h of work

comment:3 Changed 4 years ago by tmark

  • Owner set to tmark
  • Status changed from new to assigned

comment:4 Changed 4 years ago by tmark

I made the following changes:

src/lib/dhcp/tests/pkt_filter_test_stub.cc

  • PktFilterTestStub::openSocket() - the filter's SocketInfo::sock_fd_ is now set with value returend by opening "/dev/null" as read_only. This provides a valid, consumed fd that the filter retains until its socket is closed.

src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc

CtrlChannelDhcp4SrvTest removed the fd/0 work-around as it is no longer needed

All unit tests pass without issue.

I would propose the following ChangeLog:

1XXX.   [bug]   tmark
    Changed the PktFiletTestStub class, used during unit tests, to
    use the fd returned by opening "/dev/null" for its socket fd,
    rather than a hard-coded value of 0.  This eliminates test failures 
    caused when InterfaceMgr::closeSockets() is called. 
    (Trac #4067, git TBD)


comment:5 Changed 4 years ago by tmark

  • Owner changed from tmark to UnAssigned
  • Status changed from assigned to reviewing

comment:6 Changed 4 years ago by fdupont

I have a concern about the ChangeLog wording: I suggest to replace the first fd by file descriptor and to removed the second fd.
(note both /dev/null and the socket formally are not files, and perhaps it is not yet the time to suggest to use the (Boost) Asio socket_type and its associated error values in place of int and -1...)

comment:7 Changed 4 years ago by marcin

  • Owner changed from UnAssigned to marcin

comment:8 Changed 4 years ago by marcin

  • Owner changed from marcin to tmark

Reviewed commit ec3b132b582acc0a089f41b19e1dd6a6d217603d

pkt_filter_test_stub.h
I the documentation it says that it returns socket descriptor 0. This should be changed to say what it now does after this update.

ChangeLog
I was wondering if the changelog is needed at all. It can be assumed to be a user visible change because failing tests should be now passing. So this is ok. But, I think the changelog is too detailed. It would be ok to say "Fixed test failures occuring for some tests when IfaceMgr::closeSockets was called".

I ran unit tests on the build farm and didn't see any regressions on the systems on which I tried that.

comment:9 Changed 4 years ago by tmark

  • Resolution set to fixed
  • Status changed from reviewing to closed

I updated the commentary as requested.
Changes merged with git 9161867dc6a354659ae8b5115ee437ec76c1771e
Added ChangeLog? entry 1010, using your abbreviated text above.

Ticket is complete.

comment:10 Changed 4 years ago by tomek

  • Milestone changed from Kea1.0 to Kea1.0-beta

Milestone renamed

Note: See TracTickets for help on using tickets.