Re: git: ad8d33679999 - main - mmc_xpt: use strlcpy instead of strncpy

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