Re: git: ad8d33679999 - main - mmc_xpt: use strlcpy instead of strncpy
- In reply to: Warner Losh : "Re: git: ad8d33679999 - main - mmc_xpt: use strlcpy instead of strncpy"
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Tue, 01 Jul 2025 13:18:28 UTC
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