Skip to contentSkip to conversation

perf: do not call zfree() on uninitialized pointer

PATCH Applied Jiri Slaby (SUSE) 4 participants 10 messages Nov 5 – Nov 10 1 message not in the archive perf events

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

#1 lore raw

PATCH v2 1/3 perf: do not call zfree() on uninitialized pointer

In add_drm_pmu(), the check for type > PERF_PMU_TYPE_DRM_END happens before "drm" is allocated by zalloc(). The error path nevertheless called zfree(&drm), i.e. free() on an uninitialized pointer, which can crash. Remove this bogus zfree() -- nothing has been allocated yet at this point, so there is nothing to free.

Fixes this compiler warning:
In function ‘__zfree’,
inlined from ‘add_drm_pmu’ at util/drm_pmu.c:110:3,
inlined from ‘read_drm_pmus_cb’ at util/drm_pmu.c:174:10:
../lib/zalloc.c:13:9: warning: ‘drm’ may be used uninitialized [-Wmaybe-uninitialized]

   13 |         free(*ptr);
      |         ^
Assisted-by LLM
Fixes 28917cb17f9d ("perf drm_pmu: Add a tool like PMU to expose DRM information")
Cc Ian Rogers <[email protected]>

---
Cc: Peter Zijlstra <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Adrian Hunter <[email protected]>
Cc: James Clark <[email protected]>
Cc: [email protected]

---
 tools/perf/util/drm_pmu.c | 1 -
 1 file changed, 1 deletion(-)
tools/perf/util/drm_pmu.c +0 -1
index b8badae7015c..5e4bdd8e7fa8 100644
@@ -107,7 +107,6 @@ static struct drm_pmu *add_drm_pmu(struct list_head *pmus, char *line, size_t li
 		type = PERF_PMU_TYPE_DRM_START;
 
 	if (type > PERF_PMU_TYPE_DRM_END) {
-		zfree(&drm);
 		pr_err("Unable to encode DRM PMU type for %s\n", name);
 		return NULL;
 	}
Signature · 1 line
2.56.0
#2 lore raw
Sashiko has reviewed this patch and found no issues.
Quoted 11 lines

In add_drm_pmu(), the check for type > PERF_PMU_TYPE_DRM_END happens before "drm" is allocated by zalloc(). The error path nevertheless called zfree(&drm), i.e. free() on an uninitialized pointer, which can crash. Remove this bogus zfree() -- nothing has been allocated yet at this point, so there is nothing to free.

Fixes this compiler warning:
In function ‘__zfree’,
inlined from ‘add_drm_pmu’ at util/drm_pmu.c:110:3,
inlined from ‘read_drm_pmus_cb’ at util/drm_pmu.c:174:10:
../lib/zalloc.c:13:9: warning: ‘drm’ may be used uninitialized [-Wmaybe-uninitialized]

Quoted 6 lines
   13 |         free(*ptr);
      |         ^
Assisted-by LLM
Fixes 28917cb17f9d ("perf drm_pmu: Add a tool like PMU to expose DRM information")

Sashiko has reviewed this patch and found no issues.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/8b245572903b8c4ed0dc569d7a71a8f4974e2455.1793867553.git.jirislaby@kernel.org?part=1
reviewer Reviewed-by on v2 1/3
#3 lore raw
In reply to Jiri Slaby (SUSE)
Quoted 38 lines · Jiri Slaby (SUSE)

In add_drm_pmu(), the check for type > PERF_PMU_TYPE_DRM_END happens before "drm" is allocated by zalloc(). The error path nevertheless called zfree(&drm), i.e. free() on an uninitialized pointer, which can crash. Remove this bogus zfree() -- nothing has been allocated yet at this point, so there is nothing to free.

Fixes this compiler warning:
In function ‘__zfree’,
inlined from ‘add_drm_pmu’ at util/drm_pmu.c:110:3,
inlined from ‘read_drm_pmus_cb’ at util/drm_pmu.c:174:10:
../lib/zalloc.c:13:9: warning: ‘drm’ may be used uninitialized [-Wmaybe-uninitialized]

    13 |         free(*ptr);
       |         ^
Assisted-by LLM
Fixes 28917cb17f9d ("perf drm_pmu: Add a tool like PMU to expose DRM information")
Cc Ian Rogers <[email protected]>

---
Cc: Peter Zijlstra <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Adrian Hunter <[email protected]>
Cc: James Clark <[email protected]>
Cc: [email protected]

---
  tools/perf/util/drm_pmu.c | 1 -
  1 file changed, 1 deletion(-)
tools/perf/util/drm_pmu.c +0 -0
index b8badae7015c..5e4bdd8e7fa8 100644

tools/perf/util/drm_pmu.c static struct drm_pmu *add_drm_pmu(struct list_head *pmus, char *line, size_t li · line 112

Quoted 7 lines
tools/perf/util/drm_pmu.c +0 -1
@@ -107,7 +107,6 @@ static struct drm_pmu *add_drm_pmu(struct list_head *pmus, char *line, size_t li
  		type = PERF_PMU_TYPE_DRM_START;
 
  	if (type > PERF_PMU_TYPE_DRM_END) {
-		zfree(&drm);
  		pr_err("Unable to encode DRM PMU type for %s\n", name);
  		return NULL;
  	}
maintainer Applied
#4 lore raw
In reply to Jiri Slaby (SUSE)
Quoted 11 lines · Jiri Slaby (SUSE)

In add_drm_pmu(), the check for type > PERF_PMU_TYPE_DRM_END happens before "drm" is allocated by zalloc(). The error path nevertheless called zfree(&drm), i.e. free() on an uninitialized pointer, which can crash. Remove this bogus zfree() -- nothing has been allocated yet at this point, so there is nothing to free.

Fixes this compiler warning:
In function ‘__zfree’,
inlined from ‘add_drm_pmu’ at util/drm_pmu.c:110:3,
inlined from ‘read_drm_pmus_cb’ at util/drm_pmu.c:174:10:
../lib/zalloc.c:13:9: warning: ‘drm’ may be used uninitialized [-Wmaybe-uninitialized]

Quoted 4 lines
   13 |         free(*ptr);
      |         ^

[...]

Applied to perf-tools-next, thanks!

Best regards,
Namhyung

#5 lore raw

PATCH v2 2/3 perf: do not call list_for_each_entry_safe() on uninitalized list

In reply to a message not in the archive

In test__pmu_config_helpers(), when evlist__new() fails, the code jumped to the err_out label, which calls parse_events_terms__exit(&terms). At that point parse_events_terms__init(&terms) has not run yet, so the terms list head is uninitialized and list_for_each_entry_safe() walks a garbage pointer.

Fixes this compiler warning:
In function ‘parse_events_terms__exit’,
inlined from ‘test__pmu_config_helpers’ at tests/pmu.c:296:2:
util/parse-events.c:2849:2: warning: ‘terms.terms.next’ may be used uninitia tests/pmu.c: In function ‘test__pmu_config_helpers’:
tests/pmu.c:209:28: note: ‘terms’ declared here

Assisted-by LLM
Fixes 8e2ef85c66dc ("perf tests: Test evsel__set_config_if_unset() and config change tracking")
Cc James Clark <[email protected]>

---
[v2] drop terms completely (Sashiko)

Cc Peter Zijlstra <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Alexander Shishkin <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Ian Rogers <[email protected]>
Cc Adrian Hunter <[email protected]>
---
 tools/perf/tests/pmu.c | 4 ----
 1 file changed, 4 deletions(-)
tools/perf/tests/pmu.c +0 -4
index 13e8d7fa80af..b6ad2806659c 100644
@@ -206,9 +206,7 @@ static int test__pmu_config_helpers(struct test_suite *test __maybe_unused,
 {
 	const char *event = "perf-pmu-test/config=15,config1=4,krava02=170,"
 			    "krava03=1,krava11=27,krava12=1/";
-	struct parse_events_terms terms;
 	struct parse_events_error err;
-	LIST_HEAD(config_terms);
 	struct evlist *evlist;
 	struct perf_pmu *pmu;
 	struct evsel *evsel;
@@ -226,7 +224,6 @@ static int test__pmu_config_helpers(struct test_suite *test __maybe_unused,
 		goto err_out;
 	}
 
-	parse_events_terms__init(&terms);
 	ret = parse_events(evlist, event, &err);
 	if (ret) {
 		pr_debug("failed to parse event '%s', err %d\n", event, ret);
@@ -293,7 +290,6 @@ static int test__pmu_config_helpers(struct test_suite *test __maybe_unused,
 	TEST_ASSERT_EQUAL("krava23 not overwritten", (int) val, 0);
 	ret = TEST_OK;
 err_out:
-	parse_events_terms__exit(&terms);
 	evlist__put(evlist);
 	test_pmu_put(dir, pmu);
 	return ret;
Signature · 1 line
2.56.0
#6 lore raw

Re: [PATCH v2 2/3] perf: do not call list_for_each_entry_safe() on uninitalized list

Sashiko has reviewed this patch and found no issues.
Quoted 13 lines

In test__pmu_config_helpers(), when evlist__new() fails, the code jumped to the err_out label, which calls parse_events_terms__exit(&terms). At that point parse_events_terms__init(&terms) has not run yet, so the terms list head is uninitialized and list_for_each_entry_safe() walks a garbage pointer.

Fixes this compiler warning:
In function ‘parse_events_terms__exit’,
inlined from ‘test__pmu_config_helpers’ at tests/pmu.c:296:2:
util/parse-events.c:2849:2: warning: ‘terms.terms.next’ may be used uninitia tests/pmu.c: In function ‘test__pmu_config_helpers’:
tests/pmu.c:209:28: note: ‘terms’ declared here

Quoted 3 lines
Assisted-by LLM
Fixes 8e2ef85c66dc ("perf tests: Test evsel__set_config_if_unset() and config change tracking")

Sashiko has reviewed this patch and found no issues.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/8b245572903b8c4ed0dc569d7a71a8f4974e2455.1793867553.git.jirislaby@kernel.org?part=2
reviewer Reviewed-by on v2 2/3
#7 lore raw

Re: [PATCH v2 2/3] perf: do not call list_for_each_entry_safe() on uninitalized list

In reply to Jiri Slaby (SUSE)
Quoted 57 lines · Jiri Slaby (SUSE)

In test__pmu_config_helpers(), when evlist__new() fails, the code jumped to the err_out label, which calls parse_events_terms__exit(&terms). At that point parse_events_terms__init(&terms) has not run yet, so the terms list head is uninitialized and list_for_each_entry_safe() walks a garbage pointer.

Fixes this compiler warning:
In function ‘parse_events_terms__exit’,
inlined from ‘test__pmu_config_helpers’ at tests/pmu.c:296:2:
util/parse-events.c:2849:2: warning: ‘terms.terms.next’ may be used uninitia tests/pmu.c: In function ‘test__pmu_config_helpers’:
tests/pmu.c:209:28: note: ‘terms’ declared here

Assisted-by LLM
Fixes 8e2ef85c66dc ("perf tests: Test evsel__set_config_if_unset() and config change tracking")
Cc James Clark <[email protected]>

---
[v2] drop terms completely (Sashiko)

Cc Peter Zijlstra <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Alexander Shishkin <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Ian Rogers <[email protected]>
Cc Adrian Hunter <[email protected]>
---
  tools/perf/tests/pmu.c | 4 ----
  1 file changed, 4 deletions(-)
tools/perf/tests/pmu.c +0 -3
index 13e8d7fa80af..b6ad2806659c 100644
@@ -206,9 +206,7 @@ static int test__pmu_config_helpers(struct test_suite *test __maybe_unused,
  {
  	const char *event = "perf-pmu-test/config=15,config1=4,krava02=170,"
  			    "krava03=1,krava11=27,krava12=1/";
-	struct parse_events_terms terms;
  	struct parse_events_error err;
-	LIST_HEAD(config_terms);
  	struct evlist *evlist;
  	struct perf_pmu *pmu;
  	struct evsel *evsel;
@@ -226,7 +224,6 @@ static int test__pmu_config_helpers(struct test_suite *test __maybe_unused,
  		goto err_out;
  	}
 
-	parse_events_terms__init(&terms);
  	ret = parse_events(evlist, event, &err);
  	if (ret) {
  		pr_debug("failed to parse event '%s', err %d\n", event, ret);

tools/perf/tests/pmu.c static int test__pmu_config_helpers(struct test_suite *test __maybe_unused, · line 295

Quoted 7 lines
tools/perf/tests/pmu.c +0 -1
@@ -293,7 +290,6 @@ static int test__pmu_config_helpers(struct test_suite *test __maybe_unused,
  	TEST_ASSERT_EQUAL("krava23 not overwritten", (int) val, 0);
  	ret = TEST_OK;
  err_out:
-	parse_events_terms__exit(&terms);
  	evlist__put(evlist);
  	test_pmu_put(dir, pmu);
  	return ret;
#8 lore raw

PATCH v2 3/3 perf: initialize and destroy error list in test__pmu_config_helpers()

In reply to a message not in the archive

test__pmu_config_helpers() passes an uninitialized "err" to parse_events() without ever calling parse_events_error__init() on it first. Sashiko pointed this out in review:

Quoted 6 lines

When parse_events() fails and records an error via parse_events_error__handle(), list_add(&entry->list, &err->list) and the subsequent list_for_each_entry() walks in parse_events_error__print() and parse_events_error__contains() will dereference the uninitialized err.list pointers on the stack, and any allocated parse_events_error_entry objects will be leaked.

Fix it by calling parse_events_error__init(&err) before parse_events(), and by releasing the list with parse_events_error__exit(&err) on the new err_out_errlist path taken once the list has been initialized.

Assisted-by LLM
Fixes 8e2ef85c66dc ("perf tests: Test evsel__set_config_if_unset() and config change tracking")

---
[v2] this is new in v2

Cc Peter Zijlstra <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Alexander Shishkin <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Ian Rogers <[email protected]>
Cc Adrian Hunter <[email protected]>
Cc James Clark <[email protected]>
---
 tools/perf/tests/pmu.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
tools/perf/tests/pmu.c +4 -1
index b6ad2806659c..f208932590df 100644
@@ -224,13 +224,14 @@ static int test__pmu_config_helpers(struct test_suite *test __maybe_unused,
 		goto err_out;
 	}
 
+	parse_events_error__init(&err);
 	ret = parse_events(evlist, event, &err);
 	if (ret) {
 		pr_debug("failed to parse event '%s', err %d\n", event, ret);
 		parse_events_error__print(&err, event);
 		if (parse_events_error__contains(&err, "can't access trace events"))
 			ret = TEST_SKIP;
-		goto err_out;
+		goto err_out_errlist;
 	}
 	evsel = evlist__first(evlist);
 
@@ -289,6 +290,8 @@ static int test__pmu_config_helpers(struct test_suite *test __maybe_unused,
 	evsel__get_config_val(evsel, "krava23", &val);
 	TEST_ASSERT_EQUAL("krava23 not overwritten", (int) val, 0);
 	ret = TEST_OK;
+err_out_errlist:
+	parse_events_error__exit(&err);
 err_out:
 	evlist__put(evlist);
 	test_pmu_put(dir, pmu);
Signature · 1 line
2.56.0
#9 lore raw

Re: [PATCH v2 3/3] perf: initialize and destroy error list in test__pmu_config_helpers()

Sashiko has reviewed this patch and found no issues.
Quoted 10 lines

test__pmu_config_helpers() passes an uninitialized "err" to parse_events() without ever calling parse_events_error__init() on it first. Sashiko pointed this out in review:

Quoted 6 lines

When parse_events() fails and records an error via parse_events_error__handle(), list_add(&entry->list, &err->list) and the subsequent list_for_each_entry() walks in parse_events_error__print() and parse_events_error__contains() will dereference the uninitialized err.list pointers on the stack, and any allocated parse_events_error_entry objects will be leaked.

Quoted 7 lines

Fix it by calling parse_events_error__init(&err) before parse_events(), and by releasing the list with parse_events_error__exit(&err) on the new err_out_errlist path taken once the list has been initialized.

Assisted-by LLM
Fixes 8e2ef85c66dc ("perf tests: Test evsel__set_config_if_unset() and config change tracking")

Sashiko has reviewed this patch and found no issues.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/8b245572903b8c4ed0dc569d7a71a8f4974e2455.1793867553.git.jirislaby@kernel.org?part=3
reviewer Reviewed-by on v2 3/3
#10 lore raw

Re: [PATCH v2 3/3] perf: initialize and destroy error list in test__pmu_config_helpers()

In reply to Jiri Slaby (SUSE)
Quoted 56 lines · Jiri Slaby (SUSE)

test__pmu_config_helpers() passes an uninitialized "err" to parse_events() without ever calling parse_events_error__init() on it first. Sashiko pointed this out in review:

Quoted 6 lines

When parse_events() fails and records an error via parse_events_error__handle(), list_add(&entry->list, &err->list) and the subsequent list_for_each_entry() walks in parse_events_error__print() and parse_events_error__contains() will dereference the uninitialized err.list pointers on the stack, and any allocated parse_events_error_entry objects will be leaked.

Fix it by calling parse_events_error__init(&err) before parse_events(), and by releasing the list with parse_events_error__exit(&err) on the new err_out_errlist path taken once the list has been initialized.

Assisted-by LLM
Fixes 8e2ef85c66dc ("perf tests: Test evsel__set_config_if_unset() and config change tracking")

---
[v2] this is new in v2

Cc Peter Zijlstra <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Alexander Shishkin <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Ian Rogers <[email protected]>
Cc Adrian Hunter <[email protected]>
Cc James Clark <[email protected]>
---
  tools/perf/tests/pmu.c | 5 ++++-
  1 file changed, 4 insertions(+), 1 deletion(-)
tools/perf/tests/pmu.c +2 -1
index b6ad2806659c..f208932590df 100644
@@ -224,13 +224,14 @@ static int test__pmu_config_helpers(struct test_suite *test __maybe_unused,
  		goto err_out;
  	}
 
+	parse_events_error__init(&err);
  	ret = parse_events(evlist, event, &err);
  	if (ret) {
  		pr_debug("failed to parse event '%s', err %d\n", event, ret);
  		parse_events_error__print(&err, event);
  		if (parse_events_error__contains(&err, "can't access trace events"))
  			ret = TEST_SKIP;
-		goto err_out;
+		goto err_out_errlist;
  	}
  	evsel = evlist__first(evlist);
 

tools/perf/tests/pmu.c static int test__pmu_config_helpers(struct test_suite *test __maybe_unused, · line 297

Quoted 8 lines
tools/perf/tests/pmu.c +2 -0
@@ -289,6 +290,8 @@ static int test__pmu_config_helpers(struct test_suite *test __maybe_unused,
  	evsel__get_config_val(evsel, "krava23", &val);
  	TEST_ASSERT_EQUAL("krava23 not overwritten", (int) val, 0);
  	ret = TEST_OK;
+err_out_errlist:
+	parse_events_error__exit(&err);
  err_out:
  	evlist__put(evlist);
  	test_pmu_put(dir, pmu);