| Message ID | 20221010142553.776550-5-xukuohai@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix bugs found by ASAN when running selftests | expand |
On Mon, Oct 10, 2022 at 7:08 AM Xu Kuohai <xukuohai@huawei.com> wrote: > > The get_syms() function in kprobe_multi_test.c does not free the string > memory allocated by sscanf correctly. Fix it. > > Fixes: 5b6c7e5c4434 ("selftests/bpf: Add attach bench test") > Signed-off-by: Xu Kuohai <xukuohai@huawei.com> > Acked-by: Jiri Olsa <jolsa@kernel.org> > --- > .../bpf/prog_tests/kprobe_multi_test.c | 17 ++++++++--------- > 1 file changed, 8 insertions(+), 9 deletions(-) > > diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c > index d457a55ff408..07dd2c5b7f98 100644 > --- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c > +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c > @@ -360,15 +360,14 @@ static int get_syms(char ***symsp, size_t *cntp) > * to them. Filter out the current culprits - arch_cpu_idle > * and rcu_* functions. > */ > - if (!strcmp(name, "arch_cpu_idle")) > - continue; > - if (!strncmp(name, "rcu_", 4)) > - continue; > - if (!strcmp(name, "bpf_dispatcher_xdp_func")) > - continue; > - if (!strncmp(name, "__ftrace_invalid_address__", > - sizeof("__ftrace_invalid_address__") - 1)) > + if (!strcmp(name, "arch_cpu_idle") || > + !strncmp(name, "rcu_", 4) || > + !strcmp(name, "bpf_dispatcher_xdp_func") || > + !strncmp(name, "__ftrace_invalid_address__", > + sizeof("__ftrace_invalid_address__") - 1)) { > + free(name); > continue; > + } it seems cleaner if we add if (name) free(name) under error: goto label. And in the success case when we assign name to syms[cnt] we can reset name to NULL to avoid double-free. WDYT? > err = hashmap__add(map, name, NULL); > if (err) { > free(name); > @@ -394,7 +393,7 @@ static int get_syms(char ***symsp, size_t *cntp) > hashmap__free(map); > if (err) { > for (i = 0; i < cnt; i++) > - free(syms[cnt]); > + free(syms[i]); > free(syms); > } > return err; > -- > 2.30.2 >
On 10/11/2022 9:34 AM, Andrii Nakryiko wrote: > On Mon, Oct 10, 2022 at 7:08 AM Xu Kuohai <xukuohai@huawei.com> wrote: >> >> The get_syms() function in kprobe_multi_test.c does not free the string >> memory allocated by sscanf correctly. Fix it. >> >> Fixes: 5b6c7e5c4434 ("selftests/bpf: Add attach bench test") >> Signed-off-by: Xu Kuohai <xukuohai@huawei.com> >> Acked-by: Jiri Olsa <jolsa@kernel.org> >> --- >> .../bpf/prog_tests/kprobe_multi_test.c | 17 ++++++++--------- >> 1 file changed, 8 insertions(+), 9 deletions(-) >> >> diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c >> index d457a55ff408..07dd2c5b7f98 100644 >> --- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c >> +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c >> @@ -360,15 +360,14 @@ static int get_syms(char ***symsp, size_t *cntp) >> * to them. Filter out the current culprits - arch_cpu_idle >> * and rcu_* functions. >> */ >> - if (!strcmp(name, "arch_cpu_idle")) >> - continue; >> - if (!strncmp(name, "rcu_", 4)) >> - continue; >> - if (!strcmp(name, "bpf_dispatcher_xdp_func")) >> - continue; >> - if (!strncmp(name, "__ftrace_invalid_address__", >> - sizeof("__ftrace_invalid_address__") - 1)) >> + if (!strcmp(name, "arch_cpu_idle") || >> + !strncmp(name, "rcu_", 4) || >> + !strcmp(name, "bpf_dispatcher_xdp_func") || >> + !strncmp(name, "__ftrace_invalid_address__", >> + sizeof("__ftrace_invalid_address__") - 1)) { >> + free(name); >> continue; >> + } > > it seems cleaner if we add if (name) free(name) under error: goto > label. And in the success case when we assign name to syms[cnt] we can > reset name to NULL to avoid double-free. WDYT? > Fine, but since free(NULL) works perfectly, will call free(name) unconditionally, and also initialize name to NULL, and call free(name) before sscanf. > >> err = hashmap__add(map, name, NULL); >> if (err) { >> free(name); >> @@ -394,7 +393,7 @@ static int get_syms(char ***symsp, size_t *cntp) >> hashmap__free(map); >> if (err) { >> for (i = 0; i < cnt; i++) >> - free(syms[cnt]); >> + free(syms[i]); >> free(syms); >> } >> return err; >> -- >> 2.30.2 >> > .
diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c index d457a55ff408..07dd2c5b7f98 100644 --- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c @@ -360,15 +360,14 @@ static int get_syms(char ***symsp, size_t *cntp) * to them. Filter out the current culprits - arch_cpu_idle * and rcu_* functions. */ - if (!strcmp(name, "arch_cpu_idle")) - continue; - if (!strncmp(name, "rcu_", 4)) - continue; - if (!strcmp(name, "bpf_dispatcher_xdp_func")) - continue; - if (!strncmp(name, "__ftrace_invalid_address__", - sizeof("__ftrace_invalid_address__") - 1)) + if (!strcmp(name, "arch_cpu_idle") || + !strncmp(name, "rcu_", 4) || + !strcmp(name, "bpf_dispatcher_xdp_func") || + !strncmp(name, "__ftrace_invalid_address__", + sizeof("__ftrace_invalid_address__") - 1)) { + free(name); continue; + } err = hashmap__add(map, name, NULL); if (err) { free(name); @@ -394,7 +393,7 @@ static int get_syms(char ***symsp, size_t *cntp) hashmap__free(map); if (err) { for (i = 0; i < cnt; i++) - free(syms[cnt]); + free(syms[i]); free(syms); } return err;
The get_syms() function in kprobe_multi_test.c does not free the string memory allocated by sscanf correctly. Fix it. Fixes: 5b6c7e5c4434 ("selftests/bpf: Add attach bench test") Signed-off-by: Xu Kuohai <xukuohai@huawei.com> Acked-by: Jiri Olsa <jolsa@kernel.org> --- .../bpf/prog_tests/kprobe_multi_test.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)