Date: Tue, 1 Jul 2025 16:18:28 +0300 From: Andriy Gapon <avg@freebsd.org> To: Warner Losh <imp@bsdimp.com> Cc: John Baldwin <jhb@freebsd.org>, src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: ad8d33679999 - main - mmc_xpt: use strlcpy instead of strncpy Message-ID: <04134894-a3a4-43b1-ab92-f8947e046d30@freebsd.org> In-Reply-To: <CANCZdfqBDQHZwSUeiPaJdydJ85sv6K=oGnV8=Npa7BT1fT8VQA@mail.gmail.com> References: <202506270721.55R7LlB1067221@gitrepo.freebsd.org> <69858708-8e73-4b70-9f33-5176fceb1b36@FreeBSD.org> <89e23d9e-885c-4811-886a-6c1011715679@FreeBSD.org> <CANCZdfqBDQHZwSUeiPaJdydJ85sv6K=oGnV8=Npa7BT1fT8VQA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?04134894-a3a4-43b1-ab92-f8947e046d30>