Opened 4 years ago

Closed 4 years ago

#4309 closed defect (fixed)

Unused value remote_tmp in AllocEngine::reclaimExpiredLease()

Reported by: fdupont Owned by: tmark
Priority: high Milestone: Kea1.1
Component: Unclassified 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: 7 Internal?: no

Description

Coverity complains (CID 1341466) about a double assignment of remote_tmp in AllocEngine::reclaimExpiredLease() alloc_engine.cc line 1644 without use.
Reading the code it is highly suspect...

Subtickets

Change History (7)

comment:1 Changed 4 years ago by fdupont

  • Priority changed from medium to high

The Clang static analyzer complains too! Boosting priority.

comment:2 Changed 4 years ago by tmark

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

comment:3 Changed 4 years ago by tmark

  • Owner changed from tmark to Unassigned
  • Status changed from assigned to reviewing
  • Total Hours changed from 0 to 5

Analysis of the code and its history, see #3977, #3988, I simplified the logic for flagging when the lease should be removed in reclaimExpiredLease(). Simply put, if a lease is in the declined state the removal flag will be set based on whether or not the recover hook set the skip flag. Whether or not the lease is subsequently removed is determined by the reclaim_mode parameter gating the call to reclaimLeaseInDatabase(). This solution seemed to best caputure the intent gleaned from the evolution of the code.

This should also fix the coverity issue ;).

Not sure if this warrants a ChangeLog?, but if so:

10xx.   [bug]      tmark
    Corrected Coverity issue #1341466e, in alloc_engine.cc.
    (Trac #4903, git TBD)

comment:4 Changed 4 years ago by sar

  • Owner changed from Unassigned to tmark

I did a pull on 4309 and saw the branch but no changes in the branch. Did you forget to push it?

If there hasn't been a user visible change I don't think a ChangeLog? is needed is it? If you do add an entry the current suggestion has a typo (4903 instead of 4309) in case you cut and paste it. Also while I would keep the coverity issue number in the log I would also put a short description of the issue.

comment:5 Changed 4 years ago by tmark

  • Owner changed from tmark to sar

Oops, forgot to push it. Sorry, try again

comment:6 Changed 4 years ago by sar

  • Owner changed from sar to tmark
  • Total Hours changed from 5 to 6

The code changes are fine

I've made several typo level changes to the comments as well as updated the copyright field. Please do a pull and verify they are correct, then it will be ready to merge.

comment:7 Changed 4 years ago by tmark

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 6 to 7

Changes merged with git 2bc621d5c9786ad452752af5cb476bb11e160e30

I did not add a ChangeLog entry.

Ticket is complete.

Note: See TracTickets for help on using tickets.