Opened 5 years ago

Closed 5 years ago

#3712 closed defect (fixed)

Free already freed in unit tests

Reported by: fdupont Owned by: fdupont
Priority: very high Milestone: Kea0.9.1beta
Component: dhcp Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: Very High
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description (last modified by fdupont)

Got in DHCPv4 server unit tests, a select shows it is in JSONFileBackendTest, gdb in IfaceMgr? destructor.
Details:

#0  0x00007fff8efe81a3 in malloc_error_break ()
#1  0x00007fff8efd98c5 in free ()
#2  0x000000010045b87e in std::__1::__list_imp<isc::util::OptionalValue<isc::asiolink::IOAddress>, std::__1::allocator<isc::util::OptionalValue<isc::asiolink::IOAddress> > >::empty () at iface_mgr.cc:67
#3  std::__1::__list_imp<isc::util::OptionalValue<isc::asiolink::IOAddress>, std::__1::allocator<isc::util::OptionalValue<isc::asiolink::IOAddress> > >::clear () at list:708
#4  std::__1::__list_imp<isc::util::OptionalValue<isc::asiolink::IOAddress>, std::__1::allocator<isc::util::OptionalValue<isc::asiolink::IOAddress> > >::~__list_imp () at /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/list:698
#5  0x000000010045b87e in isc::dhcp::Iface::~Iface (this=0x1013158e0) at list:810
#6  0x000000010045e48d in isc::dhcp::IfaceMgr::~IfaceMgr (this=<value temporarily unavailable, due to optimizations>) at list:720
#7  0x00007fff8919e8ed in __cxa_finalize_ranges ()
#8  0x00007fff8919ebf0 in exit ()
#9  0x00007fff93df95d0 in start ()

Subtickets

Change History (14)

comment:1 Changed 5 years ago by fdupont

  • Description modified (diff)

comment:2 Changed 5 years ago by fdupont

I removed changes since 1afa4e24b0fcdd6d3a2e596663ce1102ffe2340d and the bug is no longer here.
Adding back c726bbc4eae0f576f6791c7490bfba8c30a401d9 the bug comes back, so IMHO the bug is in the 890 stuff:

It is now possible to specify whether the DHCPv4 server
should use raw sockets or IP/UDP datagram sockets to
receive and send DHCP messages. The configuration format
has been changed for the selection of interfaces on which
the DHCPv4 and DHCPv6 servers should listen. The
configuration files using an old format are incompatible
with the latest version of Kea.

Note it is on my iMac so I am trying to reproduce it on a Fedora 20 VM. Another point: it is bound to DHCPv4 config, i.e., only the JSON config file section of the DHCPv4 server unit tests fails.

comment:3 Changed 5 years ago by fdupont

It seems it is OS X specific (BTW it doesn't mean the bug is in the BSD specific code, perhaps it is not detected on Linux/g++).

comment:4 Changed 5 years ago by fdupont

Found the source of the problem: getIfaces() called for instance by CfgIface::setState() returns a *copy* of the interface list, so the copy constructor of Iface objects is called. As it doesn't alloc a new read_buffer_ when the temporary interface list is freed the read_buffer_ is freed too...
The simplest solution is to avoid Iface object copies, I suggest first to make these objects not copyable...
As it is a critical bug I bump the priority.

comment:5 Changed 5 years ago by fdupont

  • Component changed from Unclassified to dhcp
  • Defect Severity changed from N/A to Very High
  • Priority changed from medium to very high

comment:6 follow-up: Changed 5 years ago by fdupont

The obvious/trivial (but short term) solution is to remove from Iface the read_buffer_ field which is not (yet?) used.

comment:7 Changed 5 years ago by tomek

  • Milestone changed from Kea-proposed to Kea0.9.1beta

Yup, that seems critical indeed. Moving to 0.9.1beta.

comment:8 in reply to: ↑ 6 ; follow-up: Changed 5 years ago by marcin

Replying to fdupont:

The obvious/trivial (but short term) solution is to remove from Iface the read_buffer_ field which is not (yet?) used.

This field is used in the pkt_filter_bpf.cc so it can't be removed. We can try making the Iface noncopyable and the CfgIface code (my bad) should receive a const reference, instead of copying.

comment:9 in reply to: ↑ 8 ; follow-up: Changed 5 years ago by marcin

Replying to marcin:

Replying to fdupont:

The obvious/trivial (but short term) solution is to remove from Iface the read_buffer_ field which is not (yet?) used.

This field is used in the pkt_filter_bpf.cc so it can't be removed. We can try making the Iface noncopyable and the CfgIface code (my bad) should receive a const reference, instead of copying.

Making it noncopyable won't do the trick. Perhaps we should consider removing the buffer from the Iface and allocate it when needed.

comment:10 in reply to: ↑ 9 Changed 5 years ago by fdupont

Replying to marcin:

Replying to marcin:

Replying to fdupont:

The obvious/trivial (but short term) solution is to remove from Iface the read_buffer_ field which is not (yet?) used.

This field is used in the pkt_filter_bpf.cc so it can't be removed. We can try making the Iface noncopyable and the CfgIface code (my bad) should receive a const reference, instead of copying.

Making it noncopyable won't do the trick. Perhaps we should consider removing the buffer from the Iface and allocate it when needed.

=> IMHO a vector should be better (anyway far better than realloc) and it makes possible to keep the API. Of course it is not the best idea to copy Iface objects but this can be addressed at a lower priority.

comment:11 Changed 5 years ago by fdupont

  • Owner set to fdupont
  • Status changed from new to accepted

comment:12 Changed 5 years ago by fdupont

  • Owner changed from fdupont to UnAssigned
  • Status changed from accepted to reviewing

Note to change for a std::vector makes the getReadBuffer() not longer const (IMHO it should never have been const as it is clearly used to change something in the Iface object), and as a side effect the receive() method type changed, etc.
Nevertheless I can propose in trac3712 the code with std::vector, it passed tests on my iMac.

comment:13 Changed 5 years ago by marcin

  • Owner changed from UnAssigned to fdupont

I reviewed the commit a92aa66f76a497ea973e788e4b3fb6281d062468

I made one trivial change. Please pull this change and merge.

comment:14 Changed 5 years ago by fdupont

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

Merged, creating a follow up ticket to address spurious copies of Iface objects.

Note: See TracTickets for help on using tickets.