On Sat, 23 Nov 2019 17:41:29 +0900 Kusanagi Kouichi <slash@xxxxxxxxxxxxxxx> wrote: > - Use the stricter format string. > - Avoid allocating memory. > - Check sscanf return value. > Hi Kusanagi, Thanks for the patch, small comment below. > Signed-off-by: Kusanagi Kouichi <slash@xxxxxxxxxxxxxxx> > --- > lib/trace-cmd/trace-util.c | 29 +++++++++++++++-------------- > 1 file changed, 15 insertions(+), 14 deletions(-) > > diff --git a/lib/trace-cmd/trace-util.c b/lib/trace-cmd/trace-util.c > index 8aa3b6c..ef8e4f7 100644 > --- a/lib/trace-cmd/trace-util.c > +++ b/lib/trace-cmd/trace-util.c > @@ -80,39 +80,40 @@ void tracecmd_parse_proc_kallsyms(struct tep_handle *pevent, > char *func; > char *line; > char *next = NULL; > - char *addr_str; > char *mod; > char ch; > > line = strtok_r(file, "\n", &next); > while (line) { > + int n, func_start, func_end = 0, mod_start, mod_end = 0; Small nit. I usually prefer to not have so many variables defined on one line. Something more like this: int func_start, func_end = 0; int mod_start, mod_end = 0; int n; Just easier to review. The rest looks good. I'll play with it to see if there's any instances that it doesn't work. Thanks! -- Steve > + > mod = NULL; > errno = 0; > - sscanf(line, "%ms %c %ms\t[%ms", > - &addr_str, &ch, &func, &mod); > + n = sscanf(line, "%16llx %c %n%*s%n%*1[\t][%n%*s%n", > + &addr, &ch, &func_start, &func_end, &mod_start, &mod_end); > if (errno) { > - free(addr_str); > - free(func); > - free(mod); > perror("sscanf"); > return; > } > - addr = strtoull(addr_str, NULL, 16); > - free(addr_str); > > - /* truncate the extra ']' */ > - if (mod) > - mod[strlen(mod) - 1] = 0; > + if (n != 2 || !func_end) > + return; > > + func = line + func_start; > /* > * Hacks for > * - arm arch that adds a lot of bogus '$a' functions > * - x86-64 that reports per-cpu variable offsets as absolute */ > - if (func[0] != '$' && ch != 'A' && ch != 'a') > + if (func[0] != '$' && ch != 'A' && ch != 'a') { > + line[func_end] = 0; > + if (mod_end) { > + mod = line + mod_start; > + /* truncate the extra ']' */ > + line[mod_end - 1] = 0; > + } > tep_register_function(pevent, func, addr, mod); > - free(func); > - free(mod); > + } > > line = strtok_r(NULL, "\n", &next); > }
![]() |