From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9009BC4321E for ; Tue, 11 Oct 2022 06:27:16 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229544AbiJKG1O (ORCPT ); Tue, 11 Oct 2022 02:27:14 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40004 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229723AbiJKG1L (ORCPT ); Tue, 11 Oct 2022 02:27:11 -0400 Received: from szxga01-in.huawei.com (szxga01-in.huawei.com [45.249.212.187]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 82F0C87F8E; Mon, 10 Oct 2022 23:27:10 -0700 (PDT) Received: from kwepemi500013.china.huawei.com (unknown [172.30.72.56]) by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4Mmm0p3W5mzmV6W; Tue, 11 Oct 2022 14:22:34 +0800 (CST) Received: from [10.67.111.192] (10.67.111.192) by kwepemi500013.china.huawei.com (7.221.188.120) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.31; Tue, 11 Oct 2022 14:26:46 +0800 Message-ID: Date: Tue, 11 Oct 2022 14:26:45 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.9.0 Subject: Re: [PATCH bpf v3 4/6] selftest/bpf: Fix memory leak in kprobe_multi_test Content-Language: en-US To: Andrii Nakryiko CC: , , , , Alexei Starovoitov , Daniel Borkmann , Andrii Nakryiko , Martin KaFai Lau , Song Liu , Yonghong Song , John Fastabend , KP Singh , Stanislav Fomichev , Hao Luo , Jiri Olsa , Mykola Lysenko , Shuah Khan , "David S . Miller" , Jakub Kicinski , Jesper Dangaard Brouer , Kumar Kartikeya Dwivedi , Alan Maguire , Delyan Kratunov , Lorenzo Bianconi References: <20221010142553.776550-1-xukuohai@huawei.com> <20221010142553.776550-5-xukuohai@huawei.com> From: Xu Kuohai In-Reply-To: Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.67.111.192] X-ClientProxiedBy: dggems703-chm.china.huawei.com (10.3.19.180) To kwepemi500013.china.huawei.com (7.221.188.120) X-CFilter-Loop: Reflected Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org On 10/11/2022 9:34 AM, Andrii Nakryiko wrote: > On Mon, Oct 10, 2022 at 7:08 AM Xu Kuohai 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 >> Acked-by: Jiri Olsa >> --- >> .../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 >> > .