Checkpatch suggests using strscpy() instead of strncpy().
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 strncpy() does.
In this code, strscpy() and strncpy() are equivalent and do not affect
runtime behavior. strscpy() simply copies the known string value of the
variable driver_name into the fake_bridge->name variable, which also
has a fixed size.
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]>
---
drivers/staging/vme_user/vme_fake.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/vme_user/vme_fake.c b/drivers/staging/vme_user/vme_fake.c
index 0e02c194298d..09b05861017a 100644
--- a/drivers/staging/vme_user/vme_fake.c
+++ b/drivers/staging/vme_user/vme_fake.c
@@ -67,6 +67,7 @@ struct fake_driver {
unsigned long long lm_base;
u32 lm_aspace;
u32 lm_cycle;
+
void (*lm_callback[4])(void *);
void *lm_data[4];
struct tasklet_struct int_tasklet;
@@ -1091,7 +1092,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 10:29:51AM +0300, Calvince Otieno wrote:
> Checkpatch suggests using strscpy() instead of strncpy().
>
> 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 strncpy() does.
>
> In this code, strscpy() and strncpy() are equivalent and do not affect
> runtime behavior. strscpy() simply copies the known string value of the
> variable driver_name into the fake_bridge->name variable, which also
> has a fixed size.
>
> 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]>
> ---
> drivers/staging/vme_user/vme_fake.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/vme_user/vme_fake.c b/drivers/staging/vme_user/vme_fake.c
> index 0e02c194298d..09b05861017a 100644
> --- a/drivers/staging/vme_user/vme_fake.c
> +++ b/drivers/staging/vme_user/vme_fake.c
> @@ -67,6 +67,7 @@ struct fake_driver {
> unsigned long long lm_base;
> u32 lm_aspace;
> u32 lm_cycle;
> +
> void (*lm_callback[4])(void *);
> void *lm_data[4];
Why did you make this extra line change?
> struct tasklet_struct int_tasklet;
> @@ -1091,7 +1092,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))
Are you sure this change is identical? You need to document how you
have proved that.
thanks,
greg k-h
On Wed, Oct 18, 2023 at 10:29:51AM +0300, Calvince Otieno wrote:
> Checkpatch suggests using strscpy() instead of strncpy().
>
> 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 strncpy() does.
You're not replacing strncpy(), you're replacing strcpy(). There is
never a downside to replacing strcpy() with strspy() beyond that the
secure function is probably slightly slower.
>
> In this code, strscpy() and strncpy() are equivalent and do not affect
> runtime behavior. strscpy() simply copies the known string value of the
> variable driver_name into the fake_bridge->name variable, which also
> has a fixed size.
>
> While using strscpy() does not address any bugs, it is considered a better
> practice and aligns with checkpatch recommendations.
This analysis does not say where driver_name is set, or how big it is,
or what the size of the fake_bridge->name buffer is. I would like to
see that sort of analysis in the commit message.
regards,
dan carpenter