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