[PATCH v2 iproute2] ifstat: convert sprintf to snprintf

[PATCH v2 iproute2] ifstat: convert sprintf to snprintf

[Denis Kirjanov]

Use snprintf to print only valid data

v2: adjust formatting

Signed-off-by: Denis Kirjanov <dkirjanov@suse.de>
---
 misc/ifstat.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/misc/ifstat.c b/misc/ifstat.c
index 721f4914..779c2a5a 100644
--- a/misc/ifstat.c
+++ b/misc/ifstat.c
@@ -379,10 +379,10 @@ static void format_rate(FILE *fp, const unsigned long long *vals,
 		fprintf(fp, "%8llu ", vals[i]);
 
 	if (rates[i] > mega) {
-		sprintf(temp, "%uM", (unsigned int)(rates[i]/mega));
+		snprintf(temp, sizeof(temp), "%uM", (unsigned int)(rates[i]/mega));
 		fprintf(fp, "%-6s ", temp);
 	} else if (rates[i] > kilo) {
-		sprintf(temp, "%uK", (unsigned int)(rates[i]/kilo));
+		snprintf(temp, sizeof(temp), "%uK", (unsigned int)(rates[i]/kilo));
 		fprintf(fp, "%-6s ", temp);
 	} else
 		fprintf(fp, "%-6u ", (unsigned int)rates[i]);
@@ -400,10 +400,10 @@ static void format_pair(FILE *fp, const unsigned long long *vals, int i, int k)
 		fprintf(fp, "%8llu ", vals[i]);
 
 	if (vals[k] > giga) {
-		sprintf(temp, "%uM", (unsigned int)(vals[k]/mega));
+		snprintf(temp, sizeof(temp), "%uM", (unsigned int)(vals[k]/mega));
 		fprintf(fp, "%-6s ", temp);
 	} else if (vals[k] > mega) {
-		sprintf(temp, "%uK", (unsigned int)(vals[k]/kilo));
+		snprintf(temp, sizeof(temp), "%uK", (unsigned int)(vals[k]/kilo));
 		fprintf(fp, "%-6s ", temp);
 	} else
 		fprintf(fp, "%-6u ", (unsigned int)vals[k]);
@@ -675,7 +675,7 @@ static void server_loop(int fd)
 	p.fd = fd;
 	p.events = p.revents = POLLIN;
 
-	sprintf(info_source, "%d.%lu sampling_interval=%d time_const=%d",
+	snprintf(info_source, sizeof(info_source), "%d.%lu sampling_interval=%d time_const=%d",
 		getpid(), (unsigned long)random(), scan_interval/1000, time_constant/1000);
 
 	load_info();
@@ -893,7 +893,7 @@ int main(int argc, char *argv[])
 
 	sun.sun_family = AF_UNIX;
 	sun.sun_path[0] = 0;
-	sprintf(sun.sun_path+1, "ifstat%d", getuid());
+	snprintf(sun.sun_path + 1, sizeof(sun.sun_path), "ifstat%d", getuid());
 
 	if (scan_interval > 0) {
 		if (time_constant == 0)
-- 
2.30.2

Re: [PATCH v2 iproute2] ifstat: convert sprintf to snprintf

[David Ahern]

On 2/2/24 2:35 AM, Denis Kirjanov wrote:
> @@ -893,7 +893,7 @@ int main(int argc, char *argv[])
>  
>  	sun.sun_family = AF_UNIX;
>  	sun.sun_path[0] = 0;
> -	sprintf(sun.sun_path+1, "ifstat%d", getuid());
> +	snprintf(sun.sun_path + 1, sizeof(sun.sun_path), "ifstat%d", getuid());

sizeof(sun.sun_path) - 1 since the start is +1?

and that is an odd path. Stephen do you know the origin of
"\0ifstat<uid>" for the path?

Re: [PATCH v2 iproute2] ifstat: convert sprintf to snprintf

[Stephen Hemminger]

On Tue, 6 Feb 2024 18:05:52 -0700
David Ahern <dsahern@gmail.com> wrote:

> On 2/2/24 2:35 AM, Denis Kirjanov wrote:
> > @@ -893,7 +893,7 @@ int main(int argc, char *argv[])
> >  
> >  	sun.sun_family = AF_UNIX;
> >  	sun.sun_path[0] = 0;
> > -	sprintf(sun.sun_path+1, "ifstat%d", getuid());
> > +	snprintf(sun.sun_path + 1, sizeof(sun.sun_path), "ifstat%d", getuid());  
> 
> sizeof(sun.sun_path) - 1 since the start is +1?
> 
> and that is an odd path. Stephen do you know the origin of
> "\0ifstat<uid>" for the path?


Yes that is abstract unix domain socket address.
Which avoids many issues with filesystem based Unix domain sockets.

Re: [PATCH v2 iproute2] ifstat: convert sprintf to snprintf

[Stephen Hemminger]

On Fri,  2 Feb 2024 04:35:27 -0500
Denis Kirjanov <kirjanov@gmail.com> wrote:

> Use snprintf to print only valid data
> 
> v2: adjust formatting
> 
> Signed-off-by: Denis Kirjanov <dkirjanov@suse.de>
> ---

Tried this but compile failed

ifstat.c:896:2: warning: 'snprintf' size argument is too large; destination buffer has size 107, but size argument is 108 [-Wfortify-source]
        snprintf(sun.sun_path + 1, sizeof(sun.sun_path), "ifstat%d", getuid());

Re: [PATCH v2 iproute2] ifstat: convert sprintf to snprintf

[Denis Kirjanov]

On 2/10/24 23:33, Stephen Hemminger wrote:
> On Fri,  2 Feb 2024 04:35:27 -0500
> Denis Kirjanov <kirjanov@gmail.com> wrote:
> 
>> Use snprintf to print only valid data
>>
>> v2: adjust formatting
>>
>> Signed-off-by: Denis Kirjanov <dkirjanov@suse.de>
>> ---
> 
> Tried this but compile failed
> 
> ifstat.c:896:2: warning: 'snprintf' size argument is too large; destination buffer has size 107, but size argument is 108 [-Wfortify-source]
>         snprintf(sun.sun_path + 1, sizeof(sun.sun_path), "ifstat%d", getuid());

Right, this is addressed in the patch with scnprintf
 

Re: [PATCH v2 iproute2] ifstat: convert sprintf to snprintf

[Stephen Hemminger]

On Sun, 11 Feb 2024 11:39:13 +0300
Denis Kirjanov <dkirjanov@suse.de> wrote:

> On 2/10/24 23:33, Stephen Hemminger wrote:
> > On Fri,  2 Feb 2024 04:35:27 -0500
> > Denis Kirjanov <kirjanov@gmail.com> wrote:
> >   
> >> Use snprintf to print only valid data
> >>
> >> v2: adjust formatting
> >>
> >> Signed-off-by: Denis Kirjanov <dkirjanov@suse.de>
> >> ---  
> > 
> > Tried this but compile failed
> > 
> > ifstat.c:896:2: warning: 'snprintf' size argument is too large; destination buffer has size 107, but size argument is 108 [-Wfortify-source]
> >         snprintf(sun.sun_path + 1, sizeof(sun.sun_path), "ifstat%d", getuid());  
> 
> Right, this is addressed in the patch with scnprintf
>  

But I see no need to convert to scnprintf(). Scnprintf is about the return value
and almost nowhere in iproute2 uses the return value and those that to look at the
return value are checking for beyond buffer. Plus if you convert to scnprintf you
lose lots of the fortify and other analyzer checking.

Bottom line scnprintf() makes sense in kernel but not iproute2.

Re: [PATCH v2 iproute2] ifstat: convert sprintf to snprintf

[Denis Kirjanov]

On 2/11/24 20:18, Stephen Hemminger wrote:
> On Sun, 11 Feb 2024 11:39:13 +0300
> Denis Kirjanov <dkirjanov@suse.de> wrote:
> 
>> On 2/10/24 23:33, Stephen Hemminger wrote:
>>> On Fri,  2 Feb 2024 04:35:27 -0500
>>> Denis Kirjanov <kirjanov@gmail.com> wrote:
>>>   
>>>> Use snprintf to print only valid data
>>>>
>>>> v2: adjust formatting
>>>>
>>>> Signed-off-by: Denis Kirjanov <dkirjanov@suse.de>
>>>> ---  
>>>
>>> Tried this but compile failed
>>>
>>> ifstat.c:896:2: warning: 'snprintf' size argument is too large; destination buffer has size 107, but size argument is 108 [-Wfortify-source]
>>>         snprintf(sun.sun_path + 1, sizeof(sun.sun_path), "ifstat%d", getuid());  
>>
>> Right, this is addressed in the patch with scnprintf
>>  
> 
> But I see no need to convert to scnprintf(). Scnprintf is about the return value
> and almost nowhere in iproute2 uses the return value and those that to look at the
> return value are checking for beyond buffer. Plus if you convert to scnprintf you
> lose lots of the fortify and other analyzer checking.

the last line makes sense.

> 
> Bottom line scnprintf() makes sense in kernel but not iproute2.

I'll push the next version of a patch with snprintf then.
Thanks!