| Message ID | 20231005230851.3666908-6-irogers@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | clang-tools support in tools | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Thu, Oct 5, 2023 at 4:09 PM Ian Rogers <irogers@google.com> wrote: > > Found by clang-tidy: > ``` > bench/uprobe.c:98:3: warning: Use of memory after it is freed [clang-analyzer-unix.Malloc] > bench_uprobe_bpf__destroy(skel); > ``` > > Signed-off-by: Ian Rogers <irogers@google.com> I'm ok with the change but I think it won't call bench_uprobe__teardown_bpf_skel() if the setup function returns a negative value. Maybe we also need to set the err in the default case of `switch (bench)` statement. Thanks, Namhyung > --- > tools/perf/bench/uprobe.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/tools/perf/bench/uprobe.c b/tools/perf/bench/uprobe.c > index 914c0817fe8a..5c71fdc419dd 100644 > --- a/tools/perf/bench/uprobe.c > +++ b/tools/perf/bench/uprobe.c > @@ -89,6 +89,7 @@ static int bench_uprobe__setup_bpf_skel(enum bench_uprobe bench) > return err; > cleanup: > bench_uprobe_bpf__destroy(skel); > + skel = NULL; > return err; > } > > -- > 2.42.0.609.gbb76f46606-goog >
On Sun, Oct 8, 2023 at 10:51 PM Namhyung Kim <namhyung@kernel.org> wrote: > > On Thu, Oct 5, 2023 at 4:09 PM Ian Rogers <irogers@google.com> wrote: > > > > Found by clang-tidy: > > ``` > > bench/uprobe.c:98:3: warning: Use of memory after it is freed [clang-analyzer-unix.Malloc] > > bench_uprobe_bpf__destroy(skel); > > ``` > > > > Signed-off-by: Ian Rogers <irogers@google.com> > > I'm ok with the change but I think it won't call > bench_uprobe__teardown_bpf_skel() if the setup function > returns a negative value. Maybe we also need to set the > err in the default case of `switch (bench)` statement. Yes, the analysis (I'll put it below) assumes that the err can be positive yielding destroy being called twice, the second creating a use-after-free. I think it is worth cleaning the code up and making the analyzer's job easier. Thanks, Ian ``` bench/uprobe.c:98:3: warning: Use of memory after it is freed [clang-analyzer-unix.Malloc] bench_uprobe_bpf__destroy(skel); ^ tools/perf/bench/uprobe.c:197:9: note: Calling 'bench_uprobe' return bench_uprobe(argc, argv, BENCH_UPROBE__TRACE_PRINTK); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ tools/perf/bench/uprobe.c:150:6: note: 'bench' is not equal to BENCH_UPROBE__BASELINE if (bench != BENCH_UPROBE__BASELINE && bench_uprobe__setup_bpf_skel(bench) < 0) ^~~~~ tools/perf/bench/uprobe.c:150:6: note: Left side of '&&' is true tools/perf/bench/uprobe.c:150:41: note: Calling 'bench_uprobe__setup_bpf_skel' if (bench != BENCH_UPROBE__BASELINE && bench_uprobe__setup_bpf_skel(bench) < 0) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ tools/perf/bench/uprobe.c:68:7: note: 'skel' is non-null if (!skel) { ^~~~ tools/perf/bench/uprobe.c:68:2: note: Taking false branch if (!skel) { ^ tools/perf/bench/uprobe.c:74:6: note: Assuming 'err' is not equal to 0 if (err) { ^~~ tools/perf/bench/uprobe.c:74:2: note: Taking true branch if (err) { ^ tools/perf/bench/uprobe.c:76:3: note: Control jumps to line 91 goto cleanup; ^ tools/perf/bench/uprobe.c:91:2: note: Calling 'bench_uprobe_bpf__destroy' bench_uprobe_bpf__destroy(skel); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ tools/perf/util/bpf_skel/bench_uprobe.skel.h:44:6: note: Assuming 'obj' is non-null if (!obj) ^~~~ tools/perf/util/bpf_skel/bench_uprobe.skel.h:44:2: note: Taking false branch if (!obj) ^ tools/perf/util/bpf_skel/bench_uprobe.skel.h:46:6: note: Assuming field 'skeleton' is null if (obj->skeleton) ^~~~~~~~~~~~~ tools/perf/util/bpf_skel/bench_uprobe.skel.h:46:2: note: Taking false branch if (obj->skeleton) ^ tools/perf/util/bpf_skel/bench_uprobe.skel.h:48:2: note: Memory is released free(obj); ^~~~~~~~~ tools/perf/bench/uprobe.c:91:2: note: Returning; memory was released via 1st parameter bench_uprobe_bpf__destroy(skel); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ tools/perf/bench/uprobe.c:150:41: note: Returning; memory was released if (bench != BENCH_UPROBE__BASELINE && bench_uprobe__setup_bpf_skel(bench) < 0) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ tools/perf/bench/uprobe.c:150:41: note: Assuming the condition is false if (bench != BENCH_UPROBE__BASELINE && bench_uprobe__setup_bpf_skel(bench) < 0) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ tools/perf/bench/uprobe.c:150:2: note: Taking false branch if (bench != BENCH_UPROBE__BASELINE && bench_uprobe__setup_bpf_skel(bench) < 0) ^ tools/perf/bench/uprobe.c:155:14: note: Assuming 'i' is >= 'loops' for (i = 0; i < loops; i++) { ^~~~~~~~~ tools/perf/bench/uprobe.c:155:2: note: Loop condition is false. Execution continues on line 159 for (i = 0; i < loops; i++) { ^ tools/perf/bench/uprobe.c:164:2: note: Control jumps to 'case 1:' at line 169 switch (bench_format) { ^ tools/perf/bench/uprobe.c:171:3: note: Execution continues on line 179 break; ^ tools/perf/bench/uprobe.c:179:6: note: 'bench' is not equal to BENCH_UPROBE__BASELINE if (bench != BENCH_UPROBE__BASELINE) ^~~~~ tools/perf/bench/uprobe.c:179:2: note: Taking true branch if (bench != BENCH_UPROBE__BASELINE) ^ tools/perf/bench/uprobe.c:180:3: note: Calling 'bench_uprobe__teardown_bpf_skel' bench_uprobe__teardown_bpf_skel(); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ tools/perf/bench/uprobe.c:97:6: note: 'skel' is non-null if (skel) { ^~~~ tools/perf/bench/uprobe.c:97:2: note: Taking true branch if (skel) { ^ tools/perf/bench/uprobe.c:98:3: note: Use of memory after it is freed bench_uprobe_bpf__destroy(skel); ^ ~~~~ 1 warning generated. ``` > Thanks, > Namhyung > > > > --- > > tools/perf/bench/uprobe.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/tools/perf/bench/uprobe.c b/tools/perf/bench/uprobe.c > > index 914c0817fe8a..5c71fdc419dd 100644 > > --- a/tools/perf/bench/uprobe.c > > +++ b/tools/perf/bench/uprobe.c > > @@ -89,6 +89,7 @@ static int bench_uprobe__setup_bpf_skel(enum bench_uprobe bench) > > return err; > > cleanup: > > bench_uprobe_bpf__destroy(skel); > > + skel = NULL; > > return err; > > } > > > > -- > > 2.42.0.609.gbb76f46606-goog > >
diff --git a/tools/perf/bench/uprobe.c b/tools/perf/bench/uprobe.c index 914c0817fe8a..5c71fdc419dd 100644 --- a/tools/perf/bench/uprobe.c +++ b/tools/perf/bench/uprobe.c @@ -89,6 +89,7 @@ static int bench_uprobe__setup_bpf_skel(enum bench_uprobe bench) return err; cleanup: bench_uprobe_bpf__destroy(skel); + skel = NULL; return err; }
Found by clang-tidy: ``` bench/uprobe.c:98:3: warning: Use of memory after it is freed [clang-analyzer-unix.Malloc] bench_uprobe_bpf__destroy(skel); ``` Signed-off-by: Ian Rogers <irogers@google.com> --- tools/perf/bench/uprobe.c | 1 + 1 file changed, 1 insertion(+)