Opened 10 months ago

Closed 8 months ago

#5577 closed defect (fixed)

Crash on Ubuntu with legal log libloadtests

Reported by: fdupont Owned by: tomek
Priority: medium Milestone: Kea1.4
Component: hooks 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

On my Ubuntu 14.10 64 bit VM (but was signaled on another Ubuntu version):

[ RUN      ] LegalLibLoadTest.invalidType

Program received signal SIGSEGV, Segmentation fault.
isc::hooks::CalloutManager::registerCallout (this=this@entry=0x5555557bdc00, 
    name="lease4_renew", 
    callout=0x7ffff5723fa0 <lease4_renew(isc::hooks::CalloutHandle&)>)
    at callout_manager.cc:78
78	        if (i->first > current_library_) {
(gdb) p i
$1 = <error reading variable: Cannot access memory at address 0x1>
(gdb) where
#0  isc::hooks::CalloutManager::registerCallout (
    this=this@entry=0x5555557bdc00, name="lease4_renew", 
    callout=0x7ffff5723fa0 <lease4_renew(isc::hooks::CalloutHandle&)>)
    at callout_manager.cc:78
#1  0x00007ffff753eb99 in isc::hooks::LibraryHandle::registerCallout (
    this=0x5555557bdc28, name="lease4_renew", callout=<optimized out>)
    at library_handle.cc:29
#2  0x00007ffff75403c2 in isc::hooks::LibraryManager::registerStandardCallouts
    (this=this@entry=0x5555557c1970) at library_manager.cc:154
#3  0x00007ffff7541a40 in isc::hooks::LibraryManager::loadLibrary (
    this=0x5555557c1970) at library_manager.cc:283
#4  0x00007ffff7542403 in isc::hooks::LibraryManagerCollection::loadLibraries (
    this=this@entry=0x5555557c17d0) at library_manager_collection.cc:101

Subtickets

Change History (15)

comment:1 Changed 10 months ago by fdupont

Problem is simple:

void
CalloutManager::registerCallout(const std::string& name, CalloutPtr callout) {
    checkLibraryIndex(current_library_);
    int hook_index = server_hooks_.getIndex(name);
    for (CalloutVector::iterator i = hook_vector_[hook_index].begin();
         i != hook_vector_[hook_index].end(); ++i) {

The hook_index is 3 when the hook_vector_ size is 2 i.e. the callout manager was created before the server hooks (e.g. lease4_renew) were registered. Looks like a static initialization disaster...

comment:2 Changed 10 months ago by fdupont

IMHO the simplest fix is just to resize the hook_vector_ to the server_hooks_ size when new server hooks where registered. There are two places where it can be done: at registration method or (better) as need i.e. in this piece of code where the crash happens.

comment:3 Changed 10 months ago by fdupont

  • Owner set to UnAssigned
  • Status changed from new to reviewing

Fix seems to work. Ready for review.

comment:4 Changed 10 months ago by fdupont

Found the reverse (in deregisterAllCallouts()) issue. hook_index is 3 when hook_vector_ has only 2 elements.
Fixed. Note there are 5 hook_index vs hook_vector_ cases. One (last) already extended the vector, I fixed 3 of them and left the last one which raises an exception (so is not a crash case) but IMHO the whole logic must be revisited.
Ready for review.

comment:5 Changed 10 months ago by tomek

  • Milestone changed from Kea-proposed to Kea1.4

As discussed on our last call, this is indeed a problem, moving to 1.4 as medium.

comment:6 Changed 10 months ago by tomek

I confirm the fix is working. I had a problem with legal_log and radius libs. With this fix applied, the problem is now gone.

However, I disagree with the solution. I think the hook_vector_ should be resized when the new hook is being registered, not when we try to unregister callouts.

comment:7 Changed 10 months ago by fdupont

As I explained IMHO the problem is in the logic so my fixes are just to avoid crashes and do not address the deep/real problem.
If you have a better proposal I am ready to check and review it.
BTW I believe you mean to simply return on unregister when the hook_index is outside the hook_vector_?

comment:8 Changed 9 months ago by tmark

  • Owner changed from UnAssigned to tmark

comment:9 Changed 9 months ago by tmark

  • Owner changed from tmark to UnAssigned

comment:10 Changed 8 months ago by tomek

  • Owner changed from UnAssigned to tomek

Ok, this ticket seems to be stuck. Let me take a closer look at it.

comment:11 Changed 8 months ago by tomek

  • Owner changed from tomek to Unassigned

I've cleaned up the code a bit and added a unit-test. Probably more unit-tests would be useful, but given the time constrains we have to go with what we have now.

comment:12 Changed 8 months ago by tomek

The rebased change is now on trac5577_rebase.

The diff is small (154 lines). Most of which are comments.

comment:13 Changed 8 months ago by marcin

  • Owner changed from Unassigned to marcin

comment:14 Changed 8 months ago by marcin

  • Owner changed from marcin to tomek

Reviewed commit 64a967d5b18113bc81cc7c26e7465a213cf8f306

The changes look straight forward, but there are some little things I should point out.

Shouldn't ensureVectorSize be private as it is a convenience function used internally? I don't think that anyone should call this externally with a properly implemented CalloutManager class.

Secondly, the ensureVectorSize should be renamed to ensureHookLibsVectorSize because the word vector can pertain to anything. Also, the getVectorSize should probably be renamed to getHookLibsVectorSize.

Finally, you need to make sure that copyright dates in the modified files have been updated.

Apart from the last one, I am not insisting on any of those. I strongly recommend to address the second issue.

After addressing the last issue and optionally the other two you can merge the fix.

You may need a changelog entry to say that a bug fix in hooks lib has been implemented to address legal log unit test crashes on Ubuntu system.

comment:15 Changed 8 months ago by tomek

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

Addressed all problems pointed out (methods renamed, made the ensureHookLibsVectorSize private, updated copyrights, added ChangeLog?.

Thanks for the review. Closing.

Note: See TracTickets for help on using tickets.