Conversation
kknjh
requested review from
brendangregg,
chenhengqi,
ekyooo and
yonghong-song
as code owners
|
Great catch! Applying this patch as-is might be a good approach, but the issue seems to stem from how the code is structured, making such problems more likely to occur. Instead of directly applying this patch, what if we restructure the function's role? That way, the overall structure would be better, and it would be easier to maintain. I suggest modifying the What do you think? |
cmd_info and cmd_trace return directly when the parse_traces function returns a negative value, but parse_traces may also fail at the parse_trace step. Avoid such issues by restructuring the relevant functions. Signed-off-by: Feng Yang <yangfeng@kylinos.cn>
Modified, thank you. |
|
LGTM! |
libbpf-tools/ksnoop.c
Outdated
|
|
||
| nr_traces = parse_traces(argc, argv, traces); | ||
| if (nr_traces < 0) { | ||
| free(traces); |
There was a problem hiding this comment.
Dynamically allocated fields within each trace structure are not being properly cleaned up?
In parse_trace() at line 557:
trace->dump = btf_dump__new(trace->btf, trace_printf, NULL, NULL);
There was a problem hiding this comment.
Another great catch!
While it is still a memory leak like the original one, I think it would be better to handle this fix in a separate patch or PR.
Since it is happening at a separate location in the code.
Bojun-Seo
left a comment
Bojun-Seo
left a comment
There was a problem hiding this comment.
Finally, this patch series fixes three memory leaks and improves the overall resource management structure of ksnoop.
libbpf-tools/ksnoop.c
Outdated
| static void free_traces(struct trace *traces) | ||
| { | ||
| btf_dump__free(traces->dump); | ||
| btf__free(traces->btf); |
There was a problem hiding this comment.
Great catch! This is another good one.
The traces->btf is allocated via btf__load_vmlinux_btf(), which returns a struct btf * that the caller is responsible for freeing with btf__free().
Reference
https://docs.ebpf.io/ebpf-library/libbpf/userspace/btf__load_vmlinux_btf/
There was a problem hiding this comment.
Pull request overview
This PR aims to fix memory leaks in ksnoop by ensuring trace allocations are released when argument parsing fails (especially when parse_trace() fails mid-way), and by centralizing trace allocation/free logic for cmd_info and cmd_trace.
Changes:
- Refactors
parse_traces()to only parse into a caller-allocated trace array. - Adds
alloc_traces()andfree_traces()helpers and updatescmd_info/cmd_traceto use them on error paths. - Adjusts
cmd_traceto use a commoncleanup:path for better resource release.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
libbpf-tools/ksnoop.c
Outdated
| static void free_traces(struct trace *traces) | ||
| { | ||
| btf_dump__free(traces->dump); | ||
| btf__free(traces->btf); | ||
| free(traces); |
There was a problem hiding this comment.
free_traces() only frees btf/dump for traces[0] (via traces->dump / traces->btf). Each parsed trace allocates its own btf_dump (and may load a module BTF), so this will still leak resources for traces[1..] and won’t fully fix the failure path when parse_trace() fails after some traces were already parsed. Consider passing the trace count into free_traces() and iterating over all entries, freeing each trace[i].dump and any per-trace-owned BTF before freeing the array.
libbpf-tools/ksnoop.c
Outdated
| btf_dump__free(traces->dump); | ||
| btf__free(traces->btf); |
There was a problem hiding this comment.
free_traces() unconditionally calls btf__free(traces->btf), but trace->btf can be the shared global vmlinux_btf returned by get_btf(NULL). Freeing that here can leave vmlinux_btf dangling (and if free_traces is later updated to loop, it risks double-free when multiple traces use vmlinux_btf). Track ownership explicitly (e.g., only btf__free() module BTF instances, and free vmlinux_btf once at process shutdown if desired).
| btf_dump__free(traces->dump); | |
| btf__free(traces->btf); | |
| if (!traces) | |
| return; | |
| if (traces->dump) | |
| btf_dump__free(traces->dump); | |
| if (traces->btf && traces->btf != vmlinux_btf) | |
| btf__free(traces->btf); |
Add a dedicated deallocation function to include the release of data within the structure Signed-off-by: Feng Yang <yangfeng@kylinos.cn>
cmd_info and cmd_trace return directly when the parse_traces function returns a negative value, but parse_traces may also fail at the parse_trace step. Therefore, we add the release logic here to address this scenario.