On Mon, Jun 30, 2025 at 7:57 AM Andriy Gapon <a...@freebsd.org> wrote:
>
> On 30/06/2025 16:34, John Baldwin wrote:
> > On 6/27/25 03:21, Andriy Gapon wrote:
> >> The branch main has been updated by avg:
> >>
> >> URL: https://cgit.FreeBSD.org/src/commit/?
> >> id=ad8d33679999c0e7f6fd2b77d2e414102bd365ec
> >>
> >> commit ad8d33679999c0e7f6fd2b77d2e414102bd365ec
> >> Author:     Andriy Gapon <a...@freebsd.org>
> >> AuthorDate: 2025-06-23 21:31:04 +0000
> >> Commit:     Andriy Gapon <a...@freebsd.org>
> >> CommitDate: 2025-06-27 07:13:34 +0000
> >>
> >>      mmc_xpt: use strlcpy instead of strncpy
> >>      A better practice in general.
> >>      MFC after:      1 week
> >> ---
> >>   sys/cam/mmc/mmc_xpt.c | 6 +++---
> >>   1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/sys/cam/mmc/mmc_xpt.c b/sys/cam/mmc/mmc_xpt.c
> >> index 138f96eaaa49..4fce03004994 100644
> >> --- a/sys/cam/mmc/mmc_xpt.c
> >> +++ b/sys/cam/mmc/mmc_xpt.c
> >> @@ -1213,9 +1213,9 @@ mmc_path_inq(struct ccb_pathinq *cpi, const char 
> >> *hba,
> >>       cpi->max_lun = 0;
> >>       cpi->initiator_id = 1;
> >>       cpi->maxio = maxio;
> >> -    strncpy(cpi->sim_vid, "FreeBSD", SIM_IDLEN);
> >> -    strncpy(cpi->hba_vid, hba, HBA_IDLEN);
> >> -    strncpy(cpi->dev_name, cam_sim_name(sim), DEV_IDLEN);
> >> +    strlcpy(cpi->sim_vid, "FreeBSD", SIM_IDLEN);
> >> +    strlcpy(cpi->hba_vid, hba, HBA_IDLEN);
> >> +    strlcpy(cpi->dev_name, cam_sim_name(sim), DEV_IDLEN);
> >>       cpi->unit_number = cam_sim_unit(sim);
> >>       cpi->bus_id = cam_sim_bus(sim);
> >>       cpi->protocol = PROTO_MMCSD;
> >
> > Hmm, are you sure these aren't depending on strncpy zero-padding
> > the result out to the full length?  String fields in inquiry/identity
> > structures are often not C strings but have other requirements.
> > (Some of them are space padded instead of \0 padded for example.)
>
> Not sure, but using sim_vid as an example, I see strlcpy in cam_xpt and
> ctl_frontend_cam_sim.  It seems that in context of ccb_pathinq those fields 
> are
> "normal" C strings.

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.

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...

Warner

Reply via email to