Opened 5 years ago

Closed 5 years ago

#3932 closed defect (fixed)

coverity report: calling remove() without checking return value

Reported by: wlodekwencel Owned by: fdupont
Priority: low Milestone: Kea0.9.2
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: 0 Internal?: no

Description

In multiple places we call remove() without checking returned value:

     if (listen_fd_ != -1) {
            close(listen_fd_);
        }
        remove(getSocketPath().c_str());
    }

13 of those are reported in test code, coverity reports no. 1309015 to 130927 plus 2 in /src/lib/config/command_socket_factory.cc (reports no. 1309014 and 1300928)

Subtickets

Change History (8)

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

I can take it if needed. 2 comments:

  • this is not bound to the s/unlink/remove/ as both returns an int.
  • IMHO in most cases there is nothing to do, i.e., I suggest to cast to void.

BTW covert says nothing about close() which returns an int too that nobody checks... (*)

(*) there was an exception: SML NJ checked it so found the implementation of close() on Unix V9 had been wrong for years.

comment:2 in reply to: ↑ 1 Changed 5 years ago by marcin

Replying to fdupont:

I can take it if needed. 2 comments:

We should first collectively agree on the priority of this work and assign it to the appropriate milestone before this work is done. However, since this is a new stuff and it can be considered a bug, probably doing it in 0.9.2 is a good idea. But I'd check with folks on jabber first or make it a part of triage during our weekly call.

  • this is not bound to the s/unlink/remove/ as both returns an int.

For some reason coverity didn't pick unlink or maybe it was suppressed in coverity. But, I agree that this change should not have anything do it with it. But, since it pops up, it may be a good time to deal with it.

  • IMHO in most cases there is nothing to do, i.e., I suggest to cast to void.

BTW covert says nothing about close() which returns an int too that nobody checks... (*)

Some of the issues refer to the removes in unit tests in which case casting to void should be ok. Similar issues in the actual code should be examined whether the status should be checked or not. If nothing else, logging an error may be required.

(*) there was an exception: SML NJ checked it so found the implementation of close() on Unix V9 had been wrong for years.

comment:3 Changed 5 years ago by fdupont

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

comment:4 Changed 5 years ago by fdupont

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

Done. As I expected almost all calls to remove() were to be cast to void...

comment:5 Changed 5 years ago by marcin

  • Milestone changed from Kea-proposed to Kea0.9.2
  • Priority changed from medium to low

Moving to 0.9.2 per Kea call on 07/08/2015

comment:6 Changed 5 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:7 Changed 5 years ago by stephen

  • Owner changed from stephen to fdupont

Reviewed commit 1f8a452033502c934d66061574da00566ae0657a

All OK, please merge.

This is a minor change and invisible to the user. I don't think a ChangeLog entry is needed.

comment:8 Changed 5 years ago by fdupont

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

Merged. Closing.

Note: See TracTickets for help on using tickets.