From: Guoxin Pu > Sent: 02 January 2024 02:31 > > Thank you for the review. Sorry if this is the duplicated reply, as I > didn't configure my mail client to send text-only message and the > previous mail was rejected by the list. > > On 02/01/2024 05:47, David Laight wrote: > >> @@ -79,8 +79,8 @@ static int parse_subpart(struct cmdline_subpart **subpart, char *partdef) > >> goto fail; > >> } > >> > >> - length = min_t(int, next - partdef, > >> - sizeof(new_subpart->name) - 1); > >> + length = min_t(int, next - partdef + 1, > >> + sizeof(new_subpart->name)); > >> strscpy(new_subpart->name, partdef, length); > > Shouldn't that be a memcpy() with the original length? > > Since it looks as though there is something equivalent to: > > next = strchr(partdef, ','); > > just above? > > Maybe with: > > new_subpart->name[length] = '\0'; > > if the target isn't zero filled (which the strncpy() probably > > relied on.) > > Yes that would be better. But since I'm fixing the issue caused by the > mentioned commit, which was an accepted change to use strscpy instead of > strncpy and seems a part of a series of changes to do that, I think > there might be a reason the maintainers preferred strscpy over strncpy > over memcpy? Otherwise we could just revert that commit and keep using > the original strncpy + setting NULL method, and then potentially swap > strncpy with memcpy. I suspect they accepted the change without realising just how creative some of the strncpy() calls are. While strscpy() is a better function than strncpy() (or strlcpy()) extreme care has to be taken to avoid adding bugs to code that was actually fine. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
