| Message ID | 20240202093527.38376-1-dkirjanov@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Stephen Hemminger |
| Headers | show |
| Series | [v2,iproute2] ifstat: convert sprintf to snprintf | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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?
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.
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());
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
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.
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!
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)
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(-)