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));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)