[PATCH] perf disasm powerpc: handle Power10 prefixed instruction continuation lines

[PATCH] perf disasm powerpc: handle Power10 prefixed instruction continuation lines

[Athira Rajeev]

Power10 introduces prefixed instructions that are 8 bytes wide.  objdump
emits these as two lines: the first carries both the raw bytes and the
mnemonic, while the second carries only the raw bytes of the second word
with no mnemonic:

  1020900c:  0b 00 10 06  pla  r31,757280  # 102c1e2c <cctki_vi_ML_CCZ4>
  10209010:  20 8e e0 3b

disasm_line__parse_powerpc() calls disasm_line__parse() on the mnemonic
portion of the line.  disasm_line__parse() returns -1 when the name
string is empty, which causes disasm_line__new() to return NULL, which
causes symbol__parse_objdump_line() to return -1, which immediately
breaks the objdump output parsing loop in symbol__disassemble_objdump().

The result is that annotation of any function containing a prefixed
instruction stops at the continuation word and all subsequent
instructions in the function are lost.

Fix this by treating an empty mnemonic as a valid case in
disasm_line__parse_powerpc(): skip the call to disasm_line__parse() and
set the instruction name to an empty heap-allocated string instead,
allowing parsing to continue to the next line.

Reported-by: Narendra Nalli <narnalli@in.ibm.com>
Reported-by: Vijay Puliyala <vpuliyal@in.ibm.com>
Signed-off-by: Athira Rajeev <atrajeev@linux.ibm.com>
---
 tools/perf/util/disasm.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
index e263a2a8715d..8dca703a5c74 100644
--- a/tools/perf/util/disasm.c
+++ b/tools/perf/util/disasm.c
@@ -884,10 +884,24 @@ static int disasm_line__parse_powerpc(struct disasm_line *dl, struct annotate_ar
 	if (name_raw_insn[0] == '\0')
 		return -1;
 
-	if (disasm)
-		ret = disasm_line__parse(name, namep, rawp);
-	else
+	if (disasm) {
+		/*
+		 * Power10 prefixed instructions are 8 bytes and objdump emits
+		 * the second word on its own line with raw bytes but no
+		 * mnemonic, e.g.:
+		 *   1020900c:  0b 00 10 06  pla  r31,757280
+		 *   10209010:  20 8e e0 3b
+		 *
+		 * Treat a missing mnemonic as an empty instruction name rather
+		 * than a parse failure, so that parsing continues past it.
+		 */
+		if (name[0] != '\0')
+			ret = disasm_line__parse(name, namep, rawp);
+		else
+			*namep = strdup("");
+	} else {
 		*namep = "";
+	}
 
 	tmp_raw_insn = strndup(name_raw_insn, 11);
 	if (tmp_raw_insn == NULL)
-- 
2.43.0

Re: [PATCH] perf disasm powerpc: handle Power10 prefixed instruction continuation lines

[sashiko-bot]

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

New issues:
- [High] Undefined behavior and potential NULL pointer dereference when formatting Power10 prefix continuation instructions because `dl->ops.raw` is uninitialized.
- [Medium] Memory leak of the dynamically allocated empty string mnemonic on continuation lines.
- [Medium] Missing NULL check for strdup() return value leading to NULL pointer dereference on OOM.

Pre-existing issues:
- [High] Freeing a static string literal `""` in the `!disasm` case leads to an immediate crash.
--

commit a4f2ef8864fcf25d6aff270970e42d7988cba27e
Author: Athira Rajeev <atrajeev@linux.ibm.com>

    perf disasm powerpc: handle Power10 prefixed instruction continuation lines
    
    Power10 introduces prefixed instructions that are 8 bytes wide. objdump
    emits these as two lines, stopping annotation when an empty mnemonic
    is encountered. Fix this by treating empty mnemonics as valid cases in
    disasm_line__parse_powerpc().

> diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
> index 6cfdbabbb8c7d..dbf5bd54d54b2 100644
> --- a/tools/perf/util/disasm.c
> +++ b/tools/perf/util/disasm.c
> @@ -884,10 +884,24 @@ static int disasm_line__parse_powerpc(struct disasm_line *dl, struct annotate_ar
>  	if (name_raw_insn[0] == '\0')
>  		return -1;
>  
> -	if (disasm)
> -		ret = disasm_line__parse(name, namep, rawp);
> -	else
> +	if (disasm) {
> +		/*
> +		 * Power10 prefixed instructions are 8 bytes and objdump emits
> +		 * the second word on its own line with raw bytes but no
> +		 * mnemonic, e.g.:
> +		 *   1020900c:  0b 00 10 06  pla  r31,757280
> +		 *   10209010:  20 8e e0 3b
> +		 *
> +		 * Treat a missing mnemonic as an empty instruction name rather
> +		 * than a parse failure, so that parsing continues past it.
> +		 */
> +		if (name[0] != '\0')
> +			ret = disasm_line__parse(name, namep, rawp);
> +		else
> +			*namep = strdup("");

[Severity: High]
Does this code leave *rawp uninitialized? By skipping the call to
disasm_line__parse() when encountering a continuation line, *rawp
(which points to dl->ops.raw) is never set, so it remains NULL.

Later, in tools/perf/util/annotate-arch/annotate-powerpc.c:check_ppc_insn(), if
the instruction matches certain categories, arithmetic__scnprintf() passes this
NULL pointer to a %s format specifier, which can cause a segfault on musl
libc, or (null) pollution on glibc:

	return scnprintf(bf, size, "%-*s %s", max_ins_name, ins->name,
			ops->raw);

[Severity: Medium]
Will this leak the memory allocated by strdup()? When parsing a continuation
line, dl->ins.name gets an empty string. Then ins__find() calls
check_ppc_insn(), which evaluates !strlen(dl->ins.name) and does this:

	if (!strlen(dl->ins.name)) {
		sprintf(name_insn, "%d", opcode);
		dl->ins.name = strdup(name_insn);
	}

This overwrites the pointer without freeing the original empty string
allocation.

[Severity: Medium]
Does this code handle out of memory conditions safely? If strdup("") fails and
returns NULL, *namep (and thus dl->ins.name) becomes NULL. Execution then
proceeds to check_ppc_insn(), which calls strlen(dl->ins.name), leading to a
NULL pointer dereference on OOM.

> +	} else {
>  		*namep = "";

[Severity: High]
This is a pre-existing issue, but can this lead to an immediate crash?

When !disasm is true (e.g. in symbol__disassemble_raw during PowerPC data type
profiling), *namep is assigned a static string literal. When the annotation
line is later freed via disasm_line__free(), it tries to free the static string
literal:

	zfree(&dl->ins.name);

This results in a deterministic crash (free(): invalid pointer) from glibc.

> +	}
>  
>  	tmp_raw_insn = strndup(name_raw_insn, 11);
>  	if (tmp_raw_insn == NULL)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907051429.29080-1-atrajeev@linux.ibm.com?part=1