lkml.org 
[lkml]   [2020]   [Mar]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 2/2] SUNRPC: Optimize 'svc_print_xprts()'
Date
Le 25/03/2020 à 15:52, Chuck Lever a écrit :
> Hi Christophe,
>
>
>> On Mar 25, 2020, at 3:04 AM, Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:
>>
>> Using 'snprintf' is safer than 'sprintf' because it can avoid a buffer
>> overflow.
> That's true as a general statement, but how likely is such an
> overflow to occur here?
>
I guess, that it us unlikely and that the 80 chars buffer is big enough.
That is the exact reason of why I've proposed 2 patches. The first one
could happen in RL. The 2nd is more like a clean-up and is less
relevant, IMHO.
>
>> The return value can also be used to avoid a strlen a call.
> That's also true of sprintf, isn't it?

Sure.


>
>> Finally, we know where we need to copy and the length to copy, so, we
>> can save a few cycles by rearraging the code and using a memcpy instead of
>> a strcat.
> I would be OK with squashing these two patches together. I don't
> see the need to keep the two changes separated.

NP, I can resend as a V2 with your comments.
As said above, the first fixes something that could, IMHO, happen and
the 2nd is more a matter of taste and a clean-up.


>
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>> This patch should have no functionnal change.
>> We could go further, use scnprintf and write directly in the destination
>> buffer. However, this could lead to a truncated last line.
> That's exactly what this function is trying to avoid. As part of any
> change in this area, it would be good to replace the current block
> comment before this function with a Doxygen-format comment that
> documents that goal.

I'll take care of it.


>> ---
>> net/sunrpc/svc_xprt.c | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
>> index df39e7b8b06c..6df861650040 100644
>> --- a/net/sunrpc/svc_xprt.c
>> +++ b/net/sunrpc/svc_xprt.c
>> @@ -118,12 +118,12 @@ int svc_print_xprts(char *buf, int maxlen)
>> list_for_each_entry(xcl, &svc_xprt_class_list, xcl_list) {
>> int slen;
>>
>> - sprintf(tmpstr, "%s %d\n", xcl->xcl_name, xcl->xcl_max_payload);
>> - slen = strlen(tmpstr);
>> - if (len + slen >= maxlen)
>> + slen = snprintf(tmpstr, sizeof(tmpstr), "%s %d\n",
>> + xcl->xcl_name, xcl->xcl_max_payload);
>> + if (slen >= sizeof(tmpstr) || len + slen >= maxlen)
>> break;
>> + memcpy(buf + len, tmpstr, slen + 1);
>> len += slen;
>> - strcat(buf, tmpstr);
> IMO replacing the strcat makes the code harder to read, and this
> is certainly not a performance path. Can you drop that part of the
> patch?

Ok


>
>> }
>> spin_unlock(&svc_xprt_class_lock);
>>
>> --
>> 2.20.1
>>
> --
> Chuck Lever
>
>
>
>


\
 
 \ /
  Last update: 2020-03-25 20:55    [W:0.069 / U:1.048 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site