[PATCH v2 1/3] perf test: Check test suite description properly

[PATCH v2 1/3] perf test: Check test suite description properly

[Namhyung Kim]

Currently perf test checks the given string with descriptions for both
test suites and cases (subtests).  But sometimes it's confusing since
the subtests don't contain the important keyword.

I think it's better to check the suite level and run the whole suite
together if it matches description in the suite.

Before:
  $ perf test hwmon
  (no output)

After:
  $ perf test hwmon
   10: Hwmon PMU                                                       :
   10.1: Basic parsing test                                            : Ok
   10.2: Parsing without PMU name                                      : Ok
   10.3: Parsing with PMU name                                         : Ok

And keep the existing behavior when it only matches test description only.

  $ perf test "Equal cpu map"
   39.5: Equal cpu map                                                 : Ok

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/tests/builtin-test.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index 80375ca39a37a256..846c9b3a732c9b3a 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -539,6 +539,7 @@ static int __cmd_test(struct test_suite **suites, int argc, const char *argv[],
 
 		for (struct test_suite **t = suites; *t; t++, curr_suite++) {
 			int curr_test_case;
+			bool suite_matched = false;
 
 			if (!perf_test__matches(test_description(*t, -1), curr_suite, argc, argv)) {
 				/*
@@ -556,6 +557,8 @@ static int __cmd_test(struct test_suite **suites, int argc, const char *argv[],
 				}
 				if (skip)
 					continue;
+			} else {
+				suite_matched = true;
 			}
 
 			if (intlist__find(skiplist, curr_suite + 1)) {
@@ -567,10 +570,10 @@ static int __cmd_test(struct test_suite **suites, int argc, const char *argv[],
 
 			for (unsigned int run = 0; run < runs_per_test; run++) {
 				test_suite__for_each_test_case(*t, curr_test_case) {
-					if (!perf_test__matches(test_description(*t, curr_test_case),
+					if (!suite_matched &&
+					    !perf_test__matches(test_description(*t, curr_test_case),
 								curr_suite, argc, argv))
 						continue;
-
 					err = start_test(*t, curr_suite, curr_test_case,
 							 &child_tests[child_test_num++],
 							 width, pass);
-- 
2.50.0.727.gbf7dc18ff4-goog

[PATCH v2 2/3] perf test: Add libsubcmd help tests

[Namhyung Kim]

Add a set of tests for subcmd routines.  Currently it fails the last one
since there's a bug.  It'll be fixed by the next commit.

  $ perf test subcmd
   69: libsubcmd help tests                                            :
   69.1: Load subcmd names                                             : Ok
   69.2: Uniquify subcmd names                                         : Ok
   69.3: Exclude duplicate subcmd names                                : FAILED!

Reviewed-by: Ian Rogers <irogers@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/tests/Build          |   1 +
 tools/perf/tests/builtin-test.c |   1 +
 tools/perf/tests/subcmd-help.c  | 108 ++++++++++++++++++++++++++++++++
 tools/perf/tests/tests.h        |   2 +
 4 files changed, 112 insertions(+)
 create mode 100644 tools/perf/tests/subcmd-help.c

diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
index d6c35dd0de3bd6ce..3e8394be15aea4fb 100644
--- a/tools/perf/tests/Build
+++ b/tools/perf/tests/Build
@@ -69,6 +69,7 @@ perf-test-y += symbols.o
 perf-test-y += util.o
 perf-test-y += hwmon_pmu.o
 perf-test-y += tool_pmu.o
+perf-test-y += subcmd-help.o
 
 ifeq ($(SRCARCH),$(filter $(SRCARCH),x86 arm arm64 powerpc))
 perf-test-$(CONFIG_DWARF_UNWIND) += dwarf-unwind.o
diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index 846c9b3a732c9b3a..e242d56523cee171 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -139,6 +139,7 @@ static struct test_suite *generic_tests[] = {
 	&suite__event_groups,
 	&suite__symbols,
 	&suite__util,
+	&suite__subcmd_help,
 	NULL,
 };
 
diff --git a/tools/perf/tests/subcmd-help.c b/tools/perf/tests/subcmd-help.c
new file mode 100644
index 0000000000000000..2280b4c0e5e7083b
--- /dev/null
+++ b/tools/perf/tests/subcmd-help.c
@@ -0,0 +1,108 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "tests.h"
+#include <linux/compiler.h>
+#include <subcmd/help.h>
+
+static int test__load_cmdnames(struct test_suite *test __maybe_unused,
+			       int subtest __maybe_unused)
+{
+	struct cmdnames cmds = {};
+
+	add_cmdname(&cmds, "aaa", 3);
+	add_cmdname(&cmds, "foo", 3);
+	add_cmdname(&cmds, "xyz", 3);
+
+	TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds, "aaa") == 1);
+	TEST_ASSERT_VAL("wrong cmd", is_in_cmdlist(&cmds, "bar") == 0);
+	TEST_ASSERT_VAL("case sensitive", is_in_cmdlist(&cmds, "XYZ") == 0);
+
+	clean_cmdnames(&cmds);
+	return TEST_OK;
+}
+
+static int test__uniq_cmdnames(struct test_suite *test __maybe_unused,
+			       int subtest __maybe_unused)
+{
+	struct cmdnames cmds = {};
+
+	/* uniq() assumes it's sorted */
+	add_cmdname(&cmds, "aaa", 3);
+	add_cmdname(&cmds, "aaa", 3);
+	add_cmdname(&cmds, "bbb", 3);
+
+	TEST_ASSERT_VAL("invalid original size", cmds.cnt == 3);
+	/* uniquify command names (to remove second 'aaa') */
+	uniq(&cmds);
+	TEST_ASSERT_VAL("invalid final size", cmds.cnt == 2);
+
+	TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds, "aaa") == 1);
+	TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds, "bbb") == 1);
+	TEST_ASSERT_VAL("wrong cmd", is_in_cmdlist(&cmds, "ccc") == 0);
+
+	clean_cmdnames(&cmds);
+	return TEST_OK;
+}
+
+static int test__exclude_cmdnames(struct test_suite *test __maybe_unused,
+				  int subtest __maybe_unused)
+{
+	struct cmdnames cmds1 = {};
+	struct cmdnames cmds2 = {};
+
+	add_cmdname(&cmds1, "aaa", 3);
+	add_cmdname(&cmds1, "bbb", 3);
+	add_cmdname(&cmds1, "ccc", 3);
+	add_cmdname(&cmds1, "ddd", 3);
+	add_cmdname(&cmds1, "eee", 3);
+	add_cmdname(&cmds1, "fff", 3);
+	add_cmdname(&cmds1, "ggg", 3);
+	add_cmdname(&cmds1, "hhh", 3);
+	add_cmdname(&cmds1, "iii", 3);
+	add_cmdname(&cmds1, "jjj", 3);
+
+	add_cmdname(&cmds2, "bbb", 3);
+	add_cmdname(&cmds2, "eee", 3);
+	add_cmdname(&cmds2, "jjj", 3);
+
+	TEST_ASSERT_VAL("invalid original size", cmds1.cnt == 10);
+	TEST_ASSERT_VAL("invalid original size", cmds2.cnt == 3);
+
+	/* remove duplicate command names in cmds1 */
+	exclude_cmds(&cmds1, &cmds2);
+
+	TEST_ASSERT_VAL("invalid excluded size", cmds1.cnt == 7);
+	TEST_ASSERT_VAL("invalid excluded size", cmds2.cnt == 3);
+
+	/* excluded commands should not belong to cmds1 */
+	TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds1, "aaa") == 1);
+	TEST_ASSERT_VAL("wrong cmd", is_in_cmdlist(&cmds1, "bbb") == 0);
+	TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds1, "ccc") == 1);
+	TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds1, "ddd") == 1);
+	TEST_ASSERT_VAL("wrong cmd", is_in_cmdlist(&cmds1, "eee") == 0);
+	TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds1, "fff") == 1);
+	TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds1, "ggg") == 1);
+	TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds1, "hhh") == 1);
+	TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds1, "iii") == 1);
+	TEST_ASSERT_VAL("wrong cmd", is_in_cmdlist(&cmds1, "jjj") == 0);
+
+	/* they should be only in cmds2 */
+	TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds2, "bbb") == 1);
+	TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds2, "eee") == 1);
+	TEST_ASSERT_VAL("cannot find cmd", is_in_cmdlist(&cmds2, "jjj") == 1);
+
+	clean_cmdnames(&cmds1);
+	clean_cmdnames(&cmds2);
+	return TEST_OK;
+}
+
+static struct test_case tests__subcmd_help[] = {
+	TEST_CASE("Load subcmd names", load_cmdnames),
+	TEST_CASE("Uniquify subcmd names", uniq_cmdnames),
+	TEST_CASE("Exclude duplicate subcmd names", exclude_cmdnames),
+	{	.name = NULL, }
+};
+
+struct test_suite suite__subcmd_help = {
+	.desc = "libsubcmd help tests",
+	.test_cases = tests__subcmd_help,
+};
diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
index 4c128a9594413b32..97e62db8764a0537 100644
--- a/tools/perf/tests/tests.h
+++ b/tools/perf/tests/tests.h
@@ -3,6 +3,7 @@
 #define TESTS_H
 
 #include <stdbool.h>
+#include "util/debug.h"
 
 enum {
 	TEST_OK   =  0,
@@ -177,6 +178,7 @@ DECLARE_SUITE(sigtrap);
 DECLARE_SUITE(event_groups);
 DECLARE_SUITE(symbols);
 DECLARE_SUITE(util);
+DECLARE_SUITE(subcmd_help);
 
 /*
  * PowerPC and S390 do not support creation of instruction breakpoints using the
-- 
2.50.0.727.gbf7dc18ff4-goog

[PATCH v2 3/3] perf tools: Fix use-after-free in help_unknown_cmd()

[Namhyung Kim]

Currently perf aborts when it finds an invalid command.  I guess it
depends on the environment as I have some custom commands in the path.

  $ perf bad-command
  perf: 'bad-command' is not a perf-command. See 'perf --help'.
  Aborted (core dumped)

It's because the exclude_cmds() in libsubcmd has a use-after-free when
it removes some entries.  After copying one to another entry, it keeps
the pointer in the both position.  And the next copy operation will free
the later one but it's the same entry in the previous one.

For example, let's say cmds = { A, B, C, D, E } and excludes = { B, E }.

  ci  cj  ei   cmds-name  excludes
  -----------+--------------------
   0   0   0 |     A         B       :    cmp < 0, ci == cj
   1   1   0 |     B         B       :    cmp == 0
   2   1   1 |     C         E       :    cmp < 0, ci != cj

At this point, it frees cmds->names[1] and cmds->names[1] is assigned to
cmds->names[2].

   3   2   1 |     D         E       :    cmp < 0, ci != cj

Now it frees cmds->names[2] but it's the same as cmds->names[1].  So
accessing cmds->names[1] will be invalid.

This makes the subcmd tests succeed.

  $ perf test subcmd
   69: libsubcmd help tests                                            :
   69.1: Load subcmd names                                             : Ok
   69.2: Uniquify subcmd names                                         : Ok
   69.3: Exclude duplicate subcmd names                                : Ok

Fixes: 657a3efee43a ("libsubcmd: Avoid SEGV/use-after-free when commands aren't excluded")
Reviewed-by: Ian Rogers <irogers@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/lib/subcmd/help.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/tools/lib/subcmd/help.c b/tools/lib/subcmd/help.c
index 8561b0f01a247690..9ef569492560efd7 100644
--- a/tools/lib/subcmd/help.c
+++ b/tools/lib/subcmd/help.c
@@ -9,6 +9,7 @@
 #include <sys/stat.h>
 #include <unistd.h>
 #include <dirent.h>
+#include <assert.h>
 #include "subcmd-util.h"
 #include "help.h"
 #include "exec-cmd.h"
@@ -82,10 +83,11 @@ void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes)
 				ci++;
 				cj++;
 			} else {
-				zfree(&cmds->names[cj]);
-				cmds->names[cj++] = cmds->names[ci++];
+				cmds->names[cj++] = cmds->names[ci];
+				cmds->names[ci++] = NULL;
 			}
 		} else if (cmp == 0) {
+			zfree(&cmds->names[ci]);
 			ci++;
 			ei++;
 		} else if (cmp > 0) {
@@ -94,12 +96,12 @@ void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes)
 	}
 	if (ci != cj) {
 		while (ci < cmds->cnt) {
-			zfree(&cmds->names[cj]);
-			cmds->names[cj++] = cmds->names[ci++];
+			cmds->names[cj++] = cmds->names[ci];
+			cmds->names[ci++] = NULL;
 		}
 	}
 	for (ci = cj; ci < cmds->cnt; ci++)
-		zfree(&cmds->names[ci]);
+		assert(cmds->names[ci] == NULL);
 	cmds->cnt = cj;
 }
 
-- 
2.50.0.727.gbf7dc18ff4-goog

Re: [PATCH v2 1/3] perf test: Check test suite description properly

[Ian Rogers]

On Tue, Jul 1, 2025 at 1:10 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Currently perf test checks the given string with descriptions for both
> test suites and cases (subtests).  But sometimes it's confusing since
> the subtests don't contain the important keyword.
>
> I think it's better to check the suite level and run the whole suite
> together if it matches description in the suite.
>
> Before:
>   $ perf test hwmon
>   (no output)
>
> After:
>   $ perf test hwmon
>    10: Hwmon PMU                                                       :
>    10.1: Basic parsing test                                            : Ok
>    10.2: Parsing without PMU name                                      : Ok
>    10.3: Parsing with PMU name                                         : Ok
>
> And keep the existing behavior when it only matches test description only.
>
>   $ perf test "Equal cpu map"
>    39.5: Equal cpu map                                                 : Ok
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Reviewed-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/tests/builtin-test.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index 80375ca39a37a256..846c9b3a732c9b3a 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c
> @@ -539,6 +539,7 @@ static int __cmd_test(struct test_suite **suites, int argc, const char *argv[],
>
>                 for (struct test_suite **t = suites; *t; t++, curr_suite++) {
>                         int curr_test_case;
> +                       bool suite_matched = false;
>
>                         if (!perf_test__matches(test_description(*t, -1), curr_suite, argc, argv)) {
>                                 /*
> @@ -556,6 +557,8 @@ static int __cmd_test(struct test_suite **suites, int argc, const char *argv[],
>                                 }
>                                 if (skip)
>                                         continue;
> +                       } else {
> +                               suite_matched = true;
>                         }
>
>                         if (intlist__find(skiplist, curr_suite + 1)) {
> @@ -567,10 +570,10 @@ static int __cmd_test(struct test_suite **suites, int argc, const char *argv[],
>
>                         for (unsigned int run = 0; run < runs_per_test; run++) {
>                                 test_suite__for_each_test_case(*t, curr_test_case) {
> -                                       if (!perf_test__matches(test_description(*t, curr_test_case),
> +                                       if (!suite_matched &&
> +                                           !perf_test__matches(test_description(*t, curr_test_case),
>                                                                 curr_suite, argc, argv))
>                                                 continue;
> -
>                                         err = start_test(*t, curr_suite, curr_test_case,
>                                                          &child_tests[child_test_num++],
>                                                          width, pass);
> --
> 2.50.0.727.gbf7dc18ff4-goog
>

Re: [PATCH v2 1/3] perf test: Check test suite description properly

[Namhyung Kim]

On Tue, 01 Jul 2025 13:10:25 -0700, Namhyung Kim wrote:
> Currently perf test checks the given string with descriptions for both
> test suites and cases (subtests).  But sometimes it's confusing since
> the subtests don't contain the important keyword.
> 
> I think it's better to check the suite level and run the whole suite
> together if it matches description in the suite.
> 
> [...]
Applied to perf-tools-next, thanks!

Best regards,
Namhyung