On 30/06/2025 18:19, Warner Losh wrote:
You should really get changes like this reviewed. "seems like" is not
a good enough reason to do this. In fact, it's a terrible reason in
the CAM code. There are places that the strncpy use is intentional and
you will screw things up if you use strlcpy. Too many fixed width
fields in old-school storage devices that reflected into different
APIs, sadly.
That's a very good advice.
I did forget that CAM and CAM-related code is a bit more magic and special than
most of FreeBSD code. strncpy -> strlcpy looked like a "no brainer" as I did
not spot anything special about those fields.
Lesson learned.
A better reason that it's 'right' is that all but 3 other places in
the tree use strlcpy for this field. The field will be NUL padded, in
practice, since CCBs tend to be zero'd before use (though path_inq
often is done with stack storage without the usual init). And a quick
look shows that we don't use dev_name, hba_vid or sim_vid as a fixed
width field anywhere but the 'compat' code that copies the whole thing
and doesn't care. We never use any of these three fields in the kernel
for anything, apart from filling them in. We do use them in camcontrol
(and similar 3rd party programs) to report them to the user as
strings. There's other places it looks like they get injected into
geom attributes, also C strings.
A look at history shows 4195c7de2471d intentionally changed them, and
that change was reviewed. It also has the reasons why it's OK in it.
So from that perspective, this is a good change. The reasons listed
there are sound (though it's too equivocal: these fields were always
NUL terminated except when the string was the exact length of the
field, which we didn't do anywhere, but that's not important).
An even better question is: shouldn't a change like this have checked
the other 4 places, and changed them as well? If it's wrong in one
place, it might be wrong elsewhere. Or looking elsewhere would confirm
or deny that this was intentional for some reason.
But there's another consideration that 4195c7de2471d didn't think
through that I have just thought of...
--
Andriy Gapon
