Page MenuHomeFreeBSD

Fix various Coverity-detected errors in nvmecontrol
ClosedPublic

Authored by dab on Mar 27 2020, 1:34 PM.

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; 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.