perf/x86: Replace strncpy() with memcpy() for vendor string

[PATCH] perf/x86: Replace strncpy() with memcpy() for vendor string

Posted by Usman Akinyemi 3 months, 3 weeks ago

strncpy() is unsafe for fixed-size binary data as
it may not NUL-terminate and is deprecated for such
usage. Since we're copying raw CPUID register values,
memcpy() is the correct and safe choice.

Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
 tools/perf/arch/x86/util/header.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/perf/arch/x86/util/header.c b/tools/perf/arch/x86/util/header.c
index 412977f8aa83..43ba55627817 100644
--- a/tools/perf/arch/x86/util/header.c
+++ b/tools/perf/arch/x86/util/header.c
@@ -16,9 +16,9 @@ void get_cpuid_0(char *vendor, unsigned int *lvl)
 	unsigned int b, c, d;
 
 	cpuid(0, 0, lvl, &b, &c, &d);
-	strncpy(&vendor[0], (char *)(&b), 4);
-	strncpy(&vendor[4], (char *)(&d), 4);
-	strncpy(&vendor[8], (char *)(&c), 4);
+	memcpy(&vendor[0], (char *)(&b), 4);
+	memcpy(&vendor[4], (char *)(&d), 4);
+	memcpy(&vendor[8], (char *)(&c), 4);
 	vendor[12] = '\0';
 }
 
-- 
2.49.0

Re: [PATCH] perf/x86: Replace strncpy() with memcpy() for vendor string

Posted by David Laight 3 months ago

On Thu, 19 Jun 2025 03:28:43 +0530
Usman Akinyemi <usmanakinyemi202@gmail.com> wrote:

> strncpy() is unsafe for fixed-size binary data as
> it may not NUL-terminate and is deprecated for such
> usage. Since we're copying raw CPUID register values,
> memcpy() is the correct and safe choice.
> 
> Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
> ---
>  tools/perf/arch/x86/util/header.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/arch/x86/util/header.c b/tools/perf/arch/x86/util/header.c
> index 412977f8aa83..43ba55627817 100644
> --- a/tools/perf/arch/x86/util/header.c
> +++ b/tools/perf/arch/x86/util/header.c
> @@ -16,9 +16,9 @@ void get_cpuid_0(char *vendor, unsigned int *lvl)
>  	unsigned int b, c, d;
>  
>  	cpuid(0, 0, lvl, &b, &c, &d);
> -	strncpy(&vendor[0], (char *)(&b), 4);
> -	strncpy(&vendor[4], (char *)(&d), 4);
> -	strncpy(&vendor[8], (char *)(&c), 4);
> +	memcpy(&vendor[0], (char *)(&b), 4);
> +	memcpy(&vendor[4], (char *)(&d), 4);
> +	memcpy(&vendor[8], (char *)(&c), 4);

Why not:
	cpuid(0, 0, lvl, (void *)vendor, (void *)(vendor + 8), (void *)(vendor + 4));


>  	vendor[12] = '\0';
>  }
>

Re: [PATCH] perf/x86: Replace strncpy() with memcpy() for vendor string

Posted by James Clark 3 months ago

On 04/07/2025 10:20 am, David Laight wrote:
> On Thu, 19 Jun 2025 03:28:43 +0530
> Usman Akinyemi <usmanakinyemi202@gmail.com> wrote:
> 
>> strncpy() is unsafe for fixed-size binary data as
>> it may not NUL-terminate and is deprecated for such

But memcpy doesn't null terminate after the 4 chars either so I don't 
think that's a good justification. Surely you don't want null 
termination, because char *vendor is supposed to be a single string 
without extra nulls in the middle. It specifically adds a null at the 
end of the function.

>> usage. Since we're copying raw CPUID register values,
>> memcpy() is the correct and safe choice.
>>

There should be a fixes: tag here if it actually fixes something. But in 
this use case strncpy seems to behave identically to memcpy so I don't 
think we should change it. Except maybe if b,c,d have NULLs in them then 
strncpy will give you uninitialized parts where memcpy won't. But that's 
not mentioned in the commit message and presumably it doesn't happen?

>> Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
>> ---
>>   tools/perf/arch/x86/util/header.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/perf/arch/x86/util/header.c b/tools/perf/arch/x86/util/header.c
>> index 412977f8aa83..43ba55627817 100644
>> --- a/tools/perf/arch/x86/util/header.c
>> +++ b/tools/perf/arch/x86/util/header.c
>> @@ -16,9 +16,9 @@ void get_cpuid_0(char *vendor, unsigned int *lvl)
>>   	unsigned int b, c, d;
>>   
>>   	cpuid(0, 0, lvl, &b, &c, &d);
>> -	strncpy(&vendor[0], (char *)(&b), 4);
>> -	strncpy(&vendor[4], (char *)(&d), 4);
>> -	strncpy(&vendor[8], (char *)(&c), 4);
>> +	memcpy(&vendor[0], (char *)(&b), 4);
>> +	memcpy(&vendor[4], (char *)(&d), 4);
>> +	memcpy(&vendor[8], (char *)(&c), 4);
> 
> Why not:
> 	cpuid(0, 0, lvl, (void *)vendor, (void *)(vendor + 8), (void *)(vendor + 4));
> 
> 
>>   	vendor[12] = '\0';
>>   }
>>   
>

Re: [PATCH] perf/x86: Replace strncpy() with memcpy() for vendor string

Posted by Usman Akinyemi 3 months ago

On Fri, Jul 4, 2025 at 4:40 PM James Clark <james.clark@linaro.org> wrote:
>
>
>
> On 04/07/2025 10:20 am, David Laight wrote:
> > On Thu, 19 Jun 2025 03:28:43 +0530
> > Usman Akinyemi <usmanakinyemi202@gmail.com> wrote:
> >
> >> strncpy() is unsafe for fixed-size binary data as
> >> it may not NUL-terminate and is deprecated for such
>
> But memcpy doesn't null terminate after the 4 chars either so I don't
> think that's a good justification. Surely you don't want null
> termination, because char *vendor is supposed to be a single string
> without extra nulls in the middle. It specifically adds a null at the
> end of the function.
>
> >> usage. Since we're copying raw CPUID register values,
> >> memcpy() is the correct and safe choice.
> >>
>
> There should be a fixes: tag here if it actually fixes something. But in
> this use case strncpy seems to behave identically to memcpy so I don't
> think we should change it. Except maybe if b,c,d have NULLs in them then
> strncpy will give you uninitialized parts where memcpy won't. But that's
> not mentioned in the commit message and presumably it doesn't happen?

Hi James,

Thanks for the review.

What you said is true, strncpy and memcpy seem to behave identically.

I should have rephrased the commit message in a different way.
While strncpy seems to work here, firstly, it is an interface that has
been deprecated.
See -> https://github.com/KSPP/linux/issues/90.
Also, memcpy is semantically correct for copying raw data compared to
strncpy which is for string.

I am not sure if the b, c, d can have a null byte, I think using the
semantically correct function (memcpy) improves the robustness even in
cases where b, c, d have null byte.

What do you think?

Thank you.
>
> >> Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
> >> ---
> >>   tools/perf/arch/x86/util/header.c | 6 +++---
> >>   1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/tools/perf/arch/x86/util/header.c b/tools/perf/arch/x86/util/header.c
> >> index 412977f8aa83..43ba55627817 100644
> >> --- a/tools/perf/arch/x86/util/header.c
> >> +++ b/tools/perf/arch/x86/util/header.c
> >> @@ -16,9 +16,9 @@ void get_cpuid_0(char *vendor, unsigned int *lvl)
> >>      unsigned int b, c, d;
> >>
> >>      cpuid(0, 0, lvl, &b, &c, &d);
> >> -    strncpy(&vendor[0], (char *)(&b), 4);
> >> -    strncpy(&vendor[4], (char *)(&d), 4);
> >> -    strncpy(&vendor[8], (char *)(&c), 4);
> >> +    memcpy(&vendor[0], (char *)(&b), 4);
> >> +    memcpy(&vendor[4], (char *)(&d), 4);
> >> +    memcpy(&vendor[8], (char *)(&c), 4);
> >
> > Why not:
> >       cpuid(0, 0, lvl, (void *)vendor, (void *)(vendor + 8), (void *)(vendor + 4));
> >
> >
> >>      vendor[12] = '\0';
> >>   }
> >>
> >
>

Re: [PATCH] perf/x86: Replace strncpy() with memcpy() for vendor string

Posted by Usman Akinyemi 3 months ago

On Fri, Jul 4, 2025 at 6:17 PM Usman Akinyemi
<usmanakinyemi202@gmail.com> wrote:
>
> On Fri, Jul 4, 2025 at 4:40 PM James Clark <james.clark@linaro.org> wrote:
> >
> >
> >
> > On 04/07/2025 10:20 am, David Laight wrote:
> > > On Thu, 19 Jun 2025 03:28:43 +0530
> > > Usman Akinyemi <usmanakinyemi202@gmail.com> wrote:
> > >
> > >> strncpy() is unsafe for fixed-size binary data as
> > >> it may not NUL-terminate and is deprecated for such
> >
> > But memcpy doesn't null terminate after the 4 chars either so I don't
> > think that's a good justification. Surely you don't want null
> > termination, because char *vendor is supposed to be a single string
> > without extra nulls in the middle. It specifically adds a null at the
> > end of the function.
> >
> > >> usage. Since we're copying raw CPUID register values,
> > >> memcpy() is the correct and safe choice.
> > >>
> >
> > There should be a fixes: tag here if it actually fixes something. But in
> > this use case strncpy seems to behave identically to memcpy so I don't
> > think we should change it. Except maybe if b,c,d have NULLs in them then
> > strncpy will give you uninitialized parts where memcpy won't. But that's
> > not mentioned in the commit message and presumably it doesn't happen?
>
> Hi James,
>
> Thanks for the review.
>
> What you said is true, strncpy and memcpy seem to behave identically.
>
> I should have rephrased the commit message in a different way.
> While strncpy seems to work here, firstly, it is an interface that has
> been deprecated.
> See -> https://github.com/KSPP/linux/issues/90.
> Also, memcpy is semantically correct for copying raw data compared to
> strncpy which is for string.
>
> I am not sure if the b, c, d can have a null byte, I think using the
> semantically correct function (memcpy) improves the robustness even in
> cases where b, c, d have null byte.
>
> What do you think?
Hello,

This is a gentle follow-up on this patch.

I would like to know if I can send the updated patch series with the
correct commit message.

Thanks and Regards
>
> Thank you.
> >
> > >> Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
> > >> ---
> > >>   tools/perf/arch/x86/util/header.c | 6 +++---
> > >>   1 file changed, 3 insertions(+), 3 deletions(-)
> > >>
> > >> diff --git a/tools/perf/arch/x86/util/header.c b/tools/perf/arch/x86/util/header.c
> > >> index 412977f8aa83..43ba55627817 100644
> > >> --- a/tools/perf/arch/x86/util/header.c
> > >> +++ b/tools/perf/arch/x86/util/header.c
> > >> @@ -16,9 +16,9 @@ void get_cpuid_0(char *vendor, unsigned int *lvl)
> > >>      unsigned int b, c, d;
> > >>
> > >>      cpuid(0, 0, lvl, &b, &c, &d);
> > >> -    strncpy(&vendor[0], (char *)(&b), 4);
> > >> -    strncpy(&vendor[4], (char *)(&d), 4);
> > >> -    strncpy(&vendor[8], (char *)(&c), 4);
> > >> +    memcpy(&vendor[0], (char *)(&b), 4);
> > >> +    memcpy(&vendor[4], (char *)(&d), 4);
> > >> +    memcpy(&vendor[8], (char *)(&c), 4);
> > >
> > > Why not:
> > >       cpuid(0, 0, lvl, (void *)vendor, (void *)(vendor + 8), (void *)(vendor + 4));
> > >
> > >
> > >>      vendor[12] = '\0';
> > >>   }
> > >>
> > >
> >

Re: [PATCH] perf/x86: Replace strncpy() with memcpy() for vendor string

Posted by Namhyung Kim 2 months, 2 weeks ago

Hello,

On Thu, Jul 10, 2025 at 07:03:07PM +0530, Usman Akinyemi wrote:
> On Fri, Jul 4, 2025 at 6:17 PM Usman Akinyemi
> <usmanakinyemi202@gmail.com> wrote:
> >
> > On Fri, Jul 4, 2025 at 4:40 PM James Clark <james.clark@linaro.org> wrote:
> > >
> > >
> > >
> > > On 04/07/2025 10:20 am, David Laight wrote:
> > > > On Thu, 19 Jun 2025 03:28:43 +0530
> > > > Usman Akinyemi <usmanakinyemi202@gmail.com> wrote:
> > > >
> > > >> strncpy() is unsafe for fixed-size binary data as
> > > >> it may not NUL-terminate and is deprecated for such
> > >
> > > But memcpy doesn't null terminate after the 4 chars either so I don't
> > > think that's a good justification. Surely you don't want null
> > > termination, because char *vendor is supposed to be a single string
> > > without extra nulls in the middle. It specifically adds a null at the
> > > end of the function.
> > >
> > > >> usage. Since we're copying raw CPUID register values,
> > > >> memcpy() is the correct and safe choice.
> > > >>
> > >
> > > There should be a fixes: tag here if it actually fixes something. But in
> > > this use case strncpy seems to behave identically to memcpy so I don't
> > > think we should change it. Except maybe if b,c,d have NULLs in them then
> > > strncpy will give you uninitialized parts where memcpy won't. But that's
> > > not mentioned in the commit message and presumably it doesn't happen?
> >
> > Hi James,
> >
> > Thanks for the review.
> >
> > What you said is true, strncpy and memcpy seem to behave identically.
> >
> > I should have rephrased the commit message in a different way.
> > While strncpy seems to work here, firstly, it is an interface that has
> > been deprecated.
> > See -> https://github.com/KSPP/linux/issues/90.
> > Also, memcpy is semantically correct for copying raw data compared to
> > strncpy which is for string.
> >
> > I am not sure if the b, c, d can have a null byte, I think using the
> > semantically correct function (memcpy) improves the robustness even in
> > cases where b, c, d have null byte.
> >
> > What do you think?
> Hello,
> 
> This is a gentle follow-up on this patch.

Sorry for the delay.

> 
> I would like to know if I can send the updated patch series with the
> correct commit message.

I feel like the strncpy() is intentional and we don't want unexpected
NUL-termination in the middle.  If it has a NUL character then it should
be a short string and don't need the later part.

Thanks,
Namhyung

Re: [PATCH] perf/x86: Replace strncpy() with memcpy() for vendor string

Posted by Usman Akinyemi 3 months ago

On Fri, Jul 4, 2025 at 2:50 PM David Laight
<david.laight.linux@gmail.com> wrote:
>
> On Thu, 19 Jun 2025 03:28:43 +0530
> Usman Akinyemi <usmanakinyemi202@gmail.com> wrote:
>
> > strncpy() is unsafe for fixed-size binary data as
> > it may not NUL-terminate and is deprecated for such
> > usage. Since we're copying raw CPUID register values,
> > memcpy() is the correct and safe choice.
> >
> > Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
> > ---
> >  tools/perf/arch/x86/util/header.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/perf/arch/x86/util/header.c b/tools/perf/arch/x86/util/header.c
> > index 412977f8aa83..43ba55627817 100644
> > --- a/tools/perf/arch/x86/util/header.c
> > +++ b/tools/perf/arch/x86/util/header.c
> > @@ -16,9 +16,9 @@ void get_cpuid_0(char *vendor, unsigned int *lvl)
> >       unsigned int b, c, d;
> >
> >       cpuid(0, 0, lvl, &b, &c, &d);
> > -     strncpy(&vendor[0], (char *)(&b), 4);
> > -     strncpy(&vendor[4], (char *)(&d), 4);
> > -     strncpy(&vendor[8], (char *)(&c), 4);
> > +     memcpy(&vendor[0], (char *)(&b), 4);
> > +     memcpy(&vendor[4], (char *)(&d), 4);
> > +     memcpy(&vendor[8], (char *)(&c), 4);
>
> Why not:
>         cpuid(0, 0, lvl, (void *)vendor, (void *)(vendor + 8), (void *)(vendor + 4));
Hello David,

This also works well. But, I think the "memcpy" is more clean and
explanatory . I can change it to this if it is prefered.

What do you think ?

Thank you.
>
>
> >       vendor[12] = '\0';
> >  }
> >
>

Re: [PATCH] perf/x86: Replace strncpy() with memcpy() for vendor string

Posted by Usman Akinyemi 3 months, 1 week ago

On Thu, Jun 19, 2025 at 3:28 AM Usman Akinyemi
<usmanakinyemi202@gmail.com> wrote:
>
> strncpy() is unsafe for fixed-size binary data as
> it may not NUL-terminate and is deprecated for such
> usage. Since we're copying raw CPUID register values,
> memcpy() is the correct and safe choice.
>
> Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
> ---
>  tools/perf/arch/x86/util/header.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/arch/x86/util/header.c b/tools/perf/arch/x86/util/header.c
> index 412977f8aa83..43ba55627817 100644
> --- a/tools/perf/arch/x86/util/header.c
> +++ b/tools/perf/arch/x86/util/header.c
> @@ -16,9 +16,9 @@ void get_cpuid_0(char *vendor, unsigned int *lvl)
>         unsigned int b, c, d;
>
>         cpuid(0, 0, lvl, &b, &c, &d);
> -       strncpy(&vendor[0], (char *)(&b), 4);
> -       strncpy(&vendor[4], (char *)(&d), 4);
> -       strncpy(&vendor[8], (char *)(&c), 4);
> +       memcpy(&vendor[0], (char *)(&b), 4);
> +       memcpy(&vendor[4], (char *)(&d), 4);
> +       memcpy(&vendor[8], (char *)(&c), 4);
>         vendor[12] = '\0';
>  }
>
Hello,

I am just bringing attention to this patch.

I have not received any comments about it.

Thank you.
Usman Akinyemi.
> --
> 2.49.0
>

On Thu, Jun 19, 2025 at 3:28 AM Usman Akinyemi
<usmanakinyemi202@gmail.com> wrote:
>
> strncpy() is unsafe for fixed-size binary data as
> it may not NUL-terminate and is deprecated for such
> usage. Since we're copying raw CPUID register values,
> memcpy() is the correct and safe choice.
>
> Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
> ---
>  tools/perf/arch/x86/util/header.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/arch/x86/util/header.c b/tools/perf/arch/x86/util/header.c
> index 412977f8aa83..43ba55627817 100644
> --- a/tools/perf/arch/x86/util/header.c
> +++ b/tools/perf/arch/x86/util/header.c
> @@ -16,9 +16,9 @@ void get_cpuid_0(char *vendor, unsigned int *lvl)
>         unsigned int b, c, d;
>
>         cpuid(0, 0, lvl, &b, &c, &d);
> -       strncpy(&vendor[0], (char *)(&b), 4);
> -       strncpy(&vendor[4], (char *)(&d), 4);
> -       strncpy(&vendor[8], (char *)(&c), 4);
> +       memcpy(&vendor[0], (char *)(&b), 4);
> +       memcpy(&vendor[4], (char *)(&d), 4);
> +       memcpy(&vendor[8], (char *)(&c), 4);
>         vendor[12] = '\0';
>  }
>
> --
> 2.49.0
>