Opened 4 years ago

Closed 4 years ago

#4339 closed defect (fixed)

Memfile backend: lease indexes not updated when lease is updated

Reported by: marcin Owned by: marcin
Priority: high Milestone: Kea1.1
Component: database-all Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: Medium
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

We use boost multi index container as a storage for leases. The multi index container allows for specifying indexes to search for objects held within the container. In our case, there are multiple indexes we use for searching for leases by client identifier, lease address etc. When lease is inserted, updated or deleted it is necassary to update all indexes. Failing to do it, may result in various errors when searching for leases, such as not finding an existing lease or finding a lease which doesn't match search criteria.

This issue was initially spotted in the Kea 0.9.1. There were two locations where the lease was updated in the multi index container using a simple assignment:

**lease_it = *lease;

These locations were: !Memfile_LeaseMgr::updateLease4 and !LeaseFileLoader::load.

Because of this assignment the indexes were not updated leading to cryptic errors during lease allocation. In particular, an increased number of unexpected NAKs could occur.

This issue was spotted in Kea 0.9.1 and got fixed in Memfile_LeaseMgr in Kea 1.0. However, it wasn't fixed in LeaseFileLoader. As a result, after the server restart some of the leases could be incorrectly loaded or overridden, leading to NAKs when client tried to renew. This ticket should correct the existing issue and provide suitable tests.

Subtickets

Attachments (1)

trac4339-kea-0.9.1.patch (3.0 KB) - added by marcin 4 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 4 years ago by fdupont

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

comment:2 Changed 4 years ago by fdupont

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

Bug fixed. I don't know what new unit tests should be added (current tests check the last update has overwritten previous values).
Ready for review. For the ChangeLog entry: use replace() which re-index leases instead of assignment of already existing leases when a lease file is loaded.

comment:3 follow-up: Changed 4 years ago by marcin

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

Francis, I am already working on this ticket. I simply forgot to "accept" it. I have unit tests and such, so re-assigning to myself.

comment:4 in reply to: ↑ 3 Changed 4 years ago by fdupont

Replying to marcin:

Francis, I am already working on this ticket. I simply forgot to "accept" it. I have unit tests and such, so re-assigning to myself.

=> not a problem (and BTW the fix itself is easy). When it will be ready please give back the ticket to me for the review.

comment:5 Changed 4 years ago by marcin

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

The issue has been fixed and the suitable unit tests have been added. The changes to be reviewed are on the following branches:

  • trac4339-kea-0.9.1-release, which corrects the issue for Kea 0.9.1
  • trac4339a, which corrects the issue on master

Note that we'll probably not release any updated version of 0.9.1 but we may need to provide a diff to our customer. Nevertheless, please review both branches.

Proposed ChangeLog entry:

XXXX.	[bug]		marcin
	libdhcpsrv: Fixed issues with lease indexing in Memfile
	database backend.
	(Trac #4319, git abcd)

comment:6 Changed 4 years ago by fdupont

  • Owner changed from UnAssigned to fdupont

comment:7 follow-up: Changed 4 years ago by fdupont

  • Owner changed from fdupont to marcin

For the 0.9.1 patch.

I new unit tests initiateRandomLease*():

  • rand() -> random() (note util/range_utilities.h already uses random())
  • for (int i -> for (unsigned i

Note you can get empty hostnames, IMHO it is not what is wanted so:

  • hostname.resize(rand()%32); -> hostname.resize(1 + rand()%30);

Another point about random() % XXX: it returns a number between 0 and XXX - 1 but not with the same probability, e.g., with XXX = 3 the value 1 has a probability of .5 when 0 and 2 have .25.

In lease4ContainerIndexUpdate same comment about rand(). There is two i++ -> ++i too, preserve a -> preserve

Same comments for lease6ContainerIndexUpdate.

For the trac4339a patch same comments with for differences:

  • no empty hostname
  • spurious @Brief (-> @brief) after initiateRandomLease6

Jenkins is at work but should be rerun after the rand/random update.


comment:8 Changed 4 years ago by fdupont

BTW I've just run the 0.9.1 branch and the new unit tests failed as expected.
And Jenkins is happy.

comment:9 in reply to: ↑ 7 Changed 4 years ago by marcin

  • Owner changed from marcin to fdupont

Replying to fdupont:

For the 0.9.1 patch.

I new unit tests initiateRandomLease*():

  • rand() -> random() (note util/range_utilities.h already uses random())
  • for (int i -> for (unsigned i

Note you can get empty hostnames, IMHO it is not what is wanted so:

  • hostname.resize(rand()%32); -> hostname.resize(1 + rand()%30);

Another point about random() % XXX: it returns a number between 0 and XXX - 1 but not with the same probability, e.g., with XXX = 3 the value 1 has a probability of .5 when 0 and 2 have .25.

In lease4ContainerIndexUpdate same comment about rand(). There is two i++ -> ++i too, preserve a -> preserve

Same comments for lease6ContainerIndexUpdate.

For the trac4339a patch same comments with for differences:

  • no empty hostname
  • spurious @Brief (-> @brief) after initiateRandomLease6

Jenkins is at work but should be rerun after the rand/random update.


I have addressed all your review comments. Please re-review.

comment:10 Changed 4 years ago by fdupont

  • Owner changed from fdupont to marcin

I changed 'preserve a lease address' to 'preserve lease address' in both branches so please push before merging.

comment:11 Changed 4 years ago by marcin

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

Merged with commit a065144663ac716b1fa1c8c224a88aa176da9630.

Changed 4 years ago by marcin

Note: See TracTickets for help on using tickets.