Re: [PATCH v11 24/33] perf lock: Avoid segv if event is missing a callchain

sashiko-bot
2026-04-13T01:54:11Z

Sashiko AI review found 1 potential issue(s):
- [Medium] Inconsistent error handling masks memory allocation failures, leading to silent data truncation. The patch modifies `get_callstack()` to return `NULL` on `calloc()` failure, and changes the caller `report_lock_contention_begin_event()` to return `0` instead of `-ENOMEM` when a callstack cannot be obtained. While the commit message justifies this as a choice to "skip the event" rather than "fail the entire file", it conflates a normal event condition (a sample missing a callchain) with a global system resource failure (OOM). This introduces an inconsistency because other memory allocation failures within the exact same function (e.g., `lock_stat_findnew`, `thread_stat_findnew`) still correctly return `-ENOMEM` and safely abort the file processing. Silently dropping events on OOM without warning the user (unless the tool is run with `-v`) degrades the reliability of the profiling tool by presenting incomplete results as successful.
--

commit 13aefff8f4f4e8214a2931fe522b7eb9e3f8f481
Author: Ian Rogers <irogers@google.com>

perf lock: Avoid segv if event is missing a callchain

This commit adds a check to avoid a segmentation fault when a sample is
missing a callchain. It also changes the error handling in
report_lock_contention_begin_event() to skip the event rather than failing
the entire file when get_callstack() returns NULL.

> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> --- a/tools/perf/builtin-lock.c
> +++ b/tools/perf/builtin-lock.c
> @@ -938,9 +938,16 @@ static u64 *get_callstack(struct perf_sample *sample, int max_stack)
> u64 i;
> int c;
>
> + if (!sample->callchain) {
> + pr_debug("Sample unexpectedly missing callchain\n");
> + return NULL;
> + }
> +
> callstack = calloc(max_stack, sizeof(*callstack));
> - if (callstack == NULL)
> + if (callstack == NULL) {
> + pr_debug("Failed to allocate callstack\n");
> return NULL;
> + }

[ ... ]

> @@ -1059,7 +1066,7 @@ static int report_lock_contention_begin_event(struct perf_sample *sample)
> if (needs_callstack()) {
> u64 *callstack = get_callstack(sample, max_stack_depth);
> if (callstack == NULL)
> - return -ENOMEM;
> + return 0;

Because get_callstack() returns NULL for both a missing callchain and a
calloc() failure, this change makes report_lock_contention_begin_event()
return 0 in both cases.

Does this conflate a normal missing callchain with a memory exhaustion
condition?

By returning 0 on OOM, the tool will silently drop events under memory
pressure rather than aborting, which might lead to incomplete and misleading
profiling results. Other memory allocations in the same function, such as
thread_stat_findnew(), still return -ENOMEM and correctly abort.

Should the OOM case still propagate an error so the file processing aborts
consistently on resource exhaustion?

--
Sashiko AI review · https://sashiko.dev/#/patchset/20260413012227.1089445-1-irogers@google.com?part=24