[PATCH] perf sched: Fix resource leak in enable_sched_schedstats()

[PATCH] perf sched: Fix resource leak in enable_sched_schedstats()

[liujing]

From: Liu Jing <liujing@cmss.chinamobile.com>

In enable_sched_schedstats(), the FILE pointer fp is only closed when
ch == '0'. If ch is not '0' (i.e., sched_schedstats is already enabled),
the function returns 0 without calling fclose(fp), causing a resource
leak.

Fix it by adding fclose(fp) before the return statement.

Signed-off-by: Liu Jing <liujing@cmss.chinamobile.com>
---
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -4114,8 +4114,8 @@
 		*reset = 1;
 		rewind(fp);
 		putc('1', fp);
-		fclose(fp);
-	}
+	}
+	fclose(fp);
 	return 0;
 }
 

[PATCH] perf intel-pt: Fix undefined shift in intel_pt_bip()

[liujing]

From: Liu Jing <liujing@cmss.chinamobile.com>

In intel_pt_bip(), the expression `1 << id` performs a signed integer
shift where id is a uint32_t. If id >= 32, this is undefined behavior.
The bounds check `id >= INTEL_PT_BLK_ITEM_ID_CNT` occurs after the shift,
so the UB has already happened.

Fix it by moving the bounds check before the shift and using `1U << id`
to perform an unsigned shift.

Signed-off-by: Liu Jing <liujing@cmss.chinamobile.com>
---
--- a/tools/perf/util/intel-pt-decoder/intel-pt-decoder.c
+++ b/tools/perf/util/intel-pt-decoder/intel-pt-decoder.c
@@ -2060,7 +2060,6 @@
 static void intel_pt_bip(struct intel_pt_decoder *decoder)
 {
 	uint32_t id = decoder->packet.count;
-	uint32_t bit = 1 << id;
 	int pos = decoder->blk_type_pos;
 
 	if (pos < 0 || id >= INTEL_PT_BLK_ITEM_ID_CNT) {
@@ -2068,6 +2067,7 @@
 			     id, decoder->blk_type);
 		return;
 	}
+	uint32_t bit = 1U << id;
 
 	if (decoder->state.items.mask[pos] & bit) {
 		intel_pt_log("WARNING: Duplicate block item %u type %d\n",

Re: [PATCH] perf intel-pt: Fix undefined shift in intel_pt_bip()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The patch introduces a build failure by placing a variable declaration after a statement block, which violates the C90 compilation rules enforced in the perf tools build system.
--

commit 8a68d39b58963034d86457f673e830e359365de8
Author: Liu Jing <liujing@cmss.chinamobile.com>

perf intel-pt: Fix undefined shift in intel_pt_bip()

This commit aims to fix an undefined shift behavior in intel_pt_bip() by
moving the bounds check before the shift and using an unsigned shift operation.

> diff --git a/tools/perf/util/intel-pt-decoder/intel-pt-decoder.c b/tools/perf/util/intel-pt-decoder/intel-pt-decoder.c
> index e733f6b1f7ac5..9abadd0d1c108 100644
> --- a/tools/perf/util/intel-pt-decoder/intel-pt-decoder.c
> +++ b/tools/perf/util/intel-pt-decoder/intel-pt-decoder.c
> @@ -2060,7 +2060,6 @@ static void intel_pt_bbp(struct intel_pt_decoder *decoder)
>  static void intel_pt_bip(struct intel_pt_decoder *decoder)
>  {
>  	uint32_t id = decoder->packet.count;
> -	uint32_t bit = 1 << id;
>  	int pos = decoder->blk_type_pos;
>  
>  	if (pos < 0 || id >= INTEL_PT_BLK_ITEM_ID_CNT) {
> @@ -2068,6 +2067,7 @@ static void intel_pt_bip(struct intel_pt_decoder *decoder)
>  			     id, decoder->blk_type);
>  		return;
>  	}
> +	uint32_t bit = 1U << id;

[Severity: Low]
Will this cause a build failure? It looks like the declaration of the bit
variable is now placed after an if statement block within intel_pt_bip().

The perf tools build system enables -Wdeclaration-after-statement and promotes
it to an error. Could this be fixed by separating the declaration and
initialization, like declaring uint32_t bit; at the top of the function and
then assigning it after the bounds check?

>  
>  	if (decoder->state.items.mask[pos] & bit) {
>  		intel_pt_log("WARNING: Duplicate block item %u type %d\n",

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903082128.4972-2-liujing@cmss.chinamobile.com?part=1

[PATCH] perf script: Fix unsigned comparison against zero in print_bstack_flags()

[liujing]

From: Liu Jing <liujing@cmss.chinamobile.com>

In print_bstack_flags(), the variable `pos` is declared as `size_t`
(unsigned), making the check `pos < 0` always false. This means the
error return from snprintf() is never detected.

Fix it by changing the return type of bstack_event_str() to int and
the type of pos to int, so that negative return values from snprintf()
are properly checked.

Signed-off-by: Liu Jing <liujing@cmss.chinamobile.com>
---
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -952,7 +952,7 @@
 	return printed;
 }
 
-static inline size_t
+static inline int
 bstack_event_str(struct branch_entry *br, char *buf, size_t sz)
 {
 	if (!(br->flags.mispred || br->flags.predicted || br->flags.not_taken))
@@ -966,7 +966,7 @@
 static int print_bstack_flags(FILE *fp, struct branch_entry *br)
 {
 	char events[16] = { 0 };
-	size_t pos;
+	int pos;
 
 	pos = bstack_event_str(br, events, sizeof(events));
 	return fprintf(fp, "/%s/%c/%c/%d/%s/%s ",

Re: [PATCH] perf script: Fix unsigned comparison against zero in print_bstack_flags()

[sashiko-bot]

> From: Liu Jing <liujing@cmss.chinamobile.com>
> 
> In print_bstack_flags(), the variable `pos` is declared as `size_t`
> (unsigned), making the check `pos < 0` always false. This means the
> error return from snprintf() is never detected.
> 
> Fix it by changing the return type of bstack_event_str() to int and
> the type of pos to int, so that negative return values from snprintf()
> are properly checked.
> 
> Signed-off-by: Liu Jing <liujing@cmss.chinamobile.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903082128.4972-3-liujing@cmss.chinamobile.com?part=1

[PATCH] perf arm: Fix memory leak in find_all_arm_spe_pmus()

[liujing]

From: Liu Jing <liujing@cmss.chinamobile.com>

In find_all_arm_spe_pmus(), if sprintf() fails, the function returns NULL
without freeing the previously allocated arm_spe_pmus array, causing a
memory leak.

Fix it by adding free(arm_spe_pmus) before the return NULL statement.

Signed-off-by: Liu Jing <liujing@cmss.chinamobile.com>
---
--- a/tools/perf/arch/arm/util/auxtrace.c
+++ b/tools/perf/arch/arm/util/auxtrace.c
@@ -39,6 +39,7 @@
 		if (ret < 0) {
 			pr_err("sprintf failed\n");
 			*err = -ENOMEM;
+			free(arm_spe_pmus);
 			return NULL;
 		}
 

Re: [PATCH] perf arm: Fix memory leak in find_all_arm_spe_pmus()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Dereference of NULL pointer when `find_all_hisi_ptt_pmus` fails to load a matched PMU.
- [Medium] Critical allocation failure errors are silently overwritten and dropped, preventing the caller from detecting OOM states during tracer initialization.
- [Low] Missing explicit inclusion of several POSIX standard headers for libc functions and constants used in the file, which will cause compilation failures under musl libc.
--

commit 4dc56b9f02f41e7883231eea2dbc58e300bd2970
Author: Liu Jing <liujing@cmss.chinamobile.com>

perf arm: Fix memory leak in find_all_arm_spe_pmus()

In find_all_arm_spe_pmus(), if sprintf() fails, the function returns NULL
without freeing the previously allocated arm_spe_pmus array, causing a
memory leak.

> diff --git a/tools/perf/arch/arm/util/auxtrace.c b/tools/perf/arch/arm/util/auxtrace.c
> index 27bb14c8b8800..dd3e1f8189a5c 100644
> --- a/tools/perf/arch/arm/util/auxtrace.c
> +++ b/tools/perf/arch/arm/util/auxtrace.c
> @@ -39,6 +39,7 @@ static struct perf_pmu **find_all_arm_spe_pmus(int *nr_spes, int *err)
>  		if (ret < 0) {
>  			pr_err("sprintf failed\n");
>  			*err = -ENOMEM;
> +			free(arm_spe_pmus);
>  			return NULL;
>  		}

[Severity: Medium]
This is a pre-existing issue, but I noticed that critical allocation failures
from find_all_arm_spe_pmus() and find_all_hisi_ptt_pmus() are silently
swallowed:

tools/perf/arch/arm/util/auxtrace.c:auxtrace_record__init() {
    ...
	arm_spe_pmus = find_all_arm_spe_pmus(&nr_spes, err);
	hisi_ptt_pmus = find_all_hisi_ptt_pmus(&nr_ptts, err);
    ...
	/*
	 * Clear 'err' even if we haven't found an event - that way perf
	 * record can still be used even if tracers aren't present.  The NULL
	 * return value will take care of telling the infrastructure HW tracing
	 * isn't available.
	 */
	*err = 0;
	return NULL;
}

If memory allocation fails in find_all_arm_spe_pmus(), it correctly sets
*err = -ENOMEM and returns NULL. However, auxtrace_record__init()
unconditionally executes *err = 0 before returning NULL when no tracers are
loaded. Doesn't this overwrite the -ENOMEM error and falsely report to the
caller that HW tracing is simply unavailable instead of warning about resource
exhaustion?


[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential NULL
pointer dereference when find_all_hisi_ptt_pmus() fails to load a matched PMU:

tools/perf/arch/arm/util/auxtrace.c:find_all_hisi_ptt_pmus() {
    ...
	while ((dent = readdir(dir))) {
		if (strstr(dent->d_name, HISI_PTT_PMU_NAME) && idx < *nr_ptts) {
			hisi_ptt_pmus[idx] = perf_pmus__find(dent->d_name);
			if (hisi_ptt_pmus[idx])
				idx++;
		}
	}

out:
	closedir(dir);
	return hisi_ptt_pmus;
}

If perf_pmus__find() returns NULL (e.g. due to sysfs read errors), idx isn't
incremented, but the function returns without updating the out parameter
*nr_ptts to match the actual number of valid pointers (idx).

Later, the caller find_pmu_for_event() loops up to the stale nr_ptts count:

tools/perf/arch/arm/util/auxtrace.c:find_pmu_for_event() {
    ...
	for (i = 0; i < pmu_nr; i++) {
		if (evsel->core.attr.type == pmus[i]->type)
			return pmus[i];
	}
    ...
}

Will this unconditional dereference of pmus[i]->type hit the trailing NULL
entries in the array, causing a segmentation fault?


[Severity: Low]
This is a pre-existing issue, but while reviewing this file I noticed several
missing standard headers that will cause compilation failures under musl libc:

tools/perf/arch/arm/util/auxtrace.c:
    #include <dirent.h>
    #include <errno.h>
    #include <stdbool.h>
    #include <linux/coresight-pmu.h>
    #include <stdlib.h>
    #include <api/fs/fs.h>

Functions like sysconf(), sprintf(), and strstr(), and constants like PATH_MAX
are used without explicitly including <unistd.h>, <stdio.h>, <string.h>, and
<limits.h>. Could these explicit POSIX header inclusions be added?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903082128.4972-4-liujing@cmss.chinamobile.com?part=1

Re: [PATCH] perf sched: Fix resource leak in enable_sched_schedstats()

[sashiko-bot]

> From: Liu Jing <liujing@cmss.chinamobile.com>
> 
> In enable_sched_schedstats(), the FILE pointer fp is only closed when
> ch == '0'. If ch is not '0' (i.e., sched_schedstats is already enabled),
> the function returns 0 without calling fclose(fp), causing a resource
> leak.
> 
> Fix it by adding fclose(fp) before the return statement.
> 
> Signed-off-by: Liu Jing <liujing@cmss.chinamobile.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903082128.4972-1-liujing@cmss.chinamobile.com?part=1