Checkpatch suggests using strscpy() instead of strcpy().
The advantages of strscpy() are that it always adds a NUL terminator
and prevents read overflows if the source string is not properly
terminated. One potential disadvantage is that it doesn't zero pad the
string like strcpy() does.
In this specific context, both strscpy and strcpy performs the same
operation without any functional difference.
The reason for this equivalence is that the driver_name string "vme_fake"
is shorter than the size of the fake_bridge->name array which is defined
as 16 characters (struct vme_bridge {char name[VMENAMSIZ];...}). Thus,
there is no risk of buffer overflow in either case. VMENAMSIZ variable
holds a constant value of 16 (#define VMENAMSIZ 16)
The null-terminated "vme_fake" string
(static const char driver_name[] = "vme_fake";) can be safely copied into
fake_bridge->name using either strscpy or strcpy.
While using strscpy() does not address any bugs, it is considered a better
practice and aligns with checkpatch recommendations.
Signed-off-by: Calvince Otieno <[email protected]>
---
Changes in V2:
- Make the commit message more clearer.
- Remove the incident extra line change.
drivers/staging/vme_user/vme_fake.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/vme_user/vme_fake.c b/drivers/staging/vme_user/vme_fake.c
index 0e02c194298d..8ab5b43c70a6 100644
--- a/drivers/staging/vme_user/vme_fake.c
+++ b/drivers/staging/vme_user/vme_fake.c
@@ -1091,7 +1091,7 @@ static int __init fake_init(void)
tasklet_init(&fake_device->int_tasklet, fake_VIRQ_tasklet,
(unsigned long)fake_bridge);
- strcpy(fake_bridge->name, driver_name);
+ strscpy(fake_bridge->name, driver_name, sizeof(fake_bridge->name))
/* Add master windows to list */
INIT_LIST_HEAD(&fake_bridge->master_resources);
--
2.34.1
On Wed, Oct 18, 2023 at 11:53:00AM +0300, Calvince Otieno wrote:
> Checkpatch suggests using strscpy() instead of strcpy().
>
> The advantages of strscpy() are that it always adds a NUL terminator
> and prevents read overflows if the source string is not properly
> terminated.
strcpy() also always adds a NUL terminator.
With strcpy() both read overflows and write overflows are an issue but
write overflows are sooooooo much more serious that we don't worry about
read overflow.
> One potential disadvantage is that it doesn't zero pad the
> string like strcpy() does.
strcpy() does not zero pad anything. You're resending patches too
quickly. You should wait a day between resends.
>
> In this specific context, both strscpy and strcpy performs the same
> operation without any functional difference.
>
> The reason for this equivalence is that the driver_name string "vme_fake"
> is shorter than the size of the fake_bridge->name array which is defined
> as 16 characters (struct vme_bridge {char name[VMENAMSIZ];...}). Thus,
> there is no risk of buffer overflow in either case. VMENAMSIZ variable
> holds a constant value of 16 (#define VMENAMSIZ 16)
This paragraph is good and sufficient.
>
> The null-terminated "vme_fake" string
> (static const char driver_name[] = "vme_fake";) can be safely copied into
> fake_bridge->name using either strscpy or strcpy.
>
> While using strscpy() does not address any bugs, it is considered a better
> practice and aligns with checkpatch recommendations.
>
> Signed-off-by: Calvince Otieno <[email protected]>
Okay. Good. Re-write the commit message and resend it tomorrow.
regards,
dan carpenter