Page MenuHomeFreeBSD

Fix various Coverity-detected errors in nvmecontrol
ClosedPublic

Authored by dab on Mar 27 2020, 1:34 PM.
Tags
None
Referenced Files
Unknown Object (File)
Dec 29 2022, 2:46 AM
Unknown Object (File)
Dec 25 2022, 12:17 AM
Unknown Object (File)
Dec 16 2022, 2:06 PM
Unknown Object (File)
Dec 12 2022, 12:26 PM
Subscribers

Details

Summary

This fixes several Coverity-detected errors in
nvmecontrol. While in here, a couple additional errors with shift/mask
confusion that were not diagnosed by Coverity are also fixed.

CIDs addressed: 1040299, 1040300, 1403972, 1403973, 1403985,
1403988, 1403990, 1404374, 1404427, 1404469, 1404510, 1404534, 1418118

CID 1403657 (resource leak of shared library handle) was marked "intentional" in the Coverity scan database.

Test Plan

Run affected nvmecontrol commands to ensure code still
functions as intended.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

This revision is now accepted and ready to land.Mar 27 2020, 3:15 PM
sbin/nvmecontrol/identify.c
97 ↗(On Diff #69918)

You probably want _SHIFT instead of _MASK.

sbin/nvmecontrol/passthru.c
194 ↗(On Diff #69918)

This message will be misleading for a short read.

253 ↗(On Diff #69918)

I wouldn't bother with the condition.

sbin/nvmecontrol/identify.c
97 ↗(On Diff #69918)

Egad! Can't believe I fixed the PIT1 but missed the SHIFT/MASK confusion on this one. Thanks.

imp requested changes to this revision.Mar 29 2020, 11:38 PM

Honestly, don't bother to unload the sharee libraries. It's a total waste of effort.

sbin/nvmecontrol/comnd.c
293 ↗(On Diff #69918)

Why?

sbin/nvmecontrol/nvmecontrol.c
187 ↗(On Diff #69918)

This makes 0 sense to me.

sbin/nvmecontrol/passthru.c
255 ↗(On Diff #69918)

Not needed either. Just close it unconditionally.

This revision now requires changes to proceed.Mar 29 2020, 11:38 PM

Fix a couple issues pointed out in review comments.

contrib/tcpdump/README
1 ↗(On Diff #69987)

Ignore this file; I had to rebuild my environment and somehow this got marked as changed. I'll fix it before committing.

Correct diff; the previous one had some unintended files included.

dab marked 3 inline comments as done.

Address comment about check on fd > -1 not being needed in passthru.c.

dab marked an inline comment as done.Mar 30 2020, 2:55 AM
dab added inline comments.
sbin/nvmecontrol/comnd.c
293 ↗(On Diff #69918)

Loading the library leaks the handle, so I implemented the list of handles and this function to address that issue. I know that it isn't strictly needed as exiting the program will unload the libraries. I didn't experiment to see if just stashing the handles in the list is enough to quiet Coverity or if it would complain about them not being deallocated by calling dlclose as this does. Assuming it works, would that approach be more acceptable?

sbin/nvmecontrol/comnd.c
293 ↗(On Diff #69918)

No other program on the planet cleans up for things like this, at least not that I've ever encountered...
I'd rather we tag the error in Coverity as a false positive. This code buys us nothing but extra run time for no benefit.

Remove the shared library unloading code at imp@'s suggestion.

dab marked 3 inline comments as done.Mar 31 2020, 8:45 PM
dab edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Apr 1 2020, 7:21 PM
This revision was automatically updated to reflect the committed changes.