Page MenuHomeFreeBSD

ctld: Fix coverity raised issues
AbandonedPublic

Authored by thj on Apr 4 2022, 11:06 AM.

Details

Reviewers
None
Group Reviewers
Klara
NetApp
Summary

Only use strtoul when str is not NULL, this addresses a coverity
warning.

Use strlcpy rather than strncpy

Only copy the lun backend string when it is valid

Test Plan

I tested that this runs and mounts with the handbook example:
https://people.freebsd.org/~blackend/en_US.ISO8859-1/books/handbook/network-iscsi.html

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 45319
Build 42207: arc lint + arc unit

Event Timeline

thj requested review of this revision.Apr 4 2022, 11:06 AM
markj added inline comments.
usr.sbin/ctld/kernel.c
368

log_errx() never returns, so str can't be NULL here. Same below.

pstef added inline comments.
usr.sbin/ctld/kernel.c
368

Here and in the other two places you check if str is not null, but I think it would read clearer if you added the cur_port->pp = 0; to the existing conditional expression and, after an else, the cur_port->pp = strtoul(str, NULL, 0);.

Hits coverity id: 1474355 in our coverity

usr.sbin/ctld/kernel.c
368

great I will drop these patches

  • Revert "ctld: Only use strtoul when str is not NULL"
usr.sbin/ctld/kernel.c
700

I don't think serial_num or device_id is expected to be NULL terminated, i.e., they are not C strings, so the change looks wrong since it might truncate the last byte by inserting a nul terminator.

790

When can l_backend be NULL? Why don't kernel_lun_add() or kernel_lun_remove() need similar treatment?

jhb added inline comments.
usr.sbin/ctld/kernel.c
368

If log_err* aren't marked as __dead2, then adding that might help Coverity to avoid adding false positives. Hmm, both log_err() and log_errx() are marked as __dead2, so not sure why Coverity didn't grok that?

700

Confirmed by digging around in the ctl code that the destination are fixed-length buffers that do not necessarily hold C strings. We need to avoid overflowing the source as the source is a C string (hence using strncpy and not memcpy), but the target does not need a nul.

790

It is never NULL. In other places in ctld we always give it a value, e.g. in conf_verify_lun in ctld.c:

	if (lun->l_backend == NULL)
		lun_set_backend(lun, "block");
usr.sbin/ctld/kernel.c
700

strncpy is the right thing here because it's a fixed length string. That's what it was written for. Same for model, etc.

707

also a fixed length string, so strncpy is right.