Messages in this thread | | | Date | Tue, 11 Oct 2022 14:26:45 +0800 | Subject | Re: [PATCH bpf v3 4/6] selftest/bpf: Fix memory leak in kprobe_multi_test | From | Xu Kuohai <> |
| |
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 >> > .
| |