On Wed, Mar 25 2020, Christophe JAILLET wrote:
> Le 25/03/2020 à 15:52, Chuck Lever a écrit :
>> Hi Christophe,
>>
>>
>>> On Mar 25, 2020, at 3:04 AM, Christophe JAILLET <[email protected]> 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 <[email protected]>
>>> ---
>>> 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);
>>>
>>> --
Can I suggest something more like this:
diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index de3c077733a7..0292f45b70f6 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -115,16 +115,9 @@ int svc_print_xprts(char *buf, int maxlen)
buf[0] = '\0';
spin_lock(&svc_xprt_class_lock);
- 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)
- break;
- len += slen;
- strcat(buf, tmpstr);
- }
+ list_for_each_entry(xcl, &svc_xprt_class_list, xcl_list)
+ len += scnprintf(buf + len, maxlen - len, "%s %d\n",
+ xcl->xcl_name, xcl->xcl_max_payload);
spin_unlock(&svc_xprt_class_lock);
return len;
NeilBrown