Re: [PATCH v1 39/58] perf intel-pt-events: Port intel-pt-events/libxed to use python module

sashiko-bot
2026-04-20T00:32:25Z

Sashiko AI review found 8 potential issue(s):
- [High] Accessing fields like `sample.cpu`, `sample.time`, `sample.pid`, and `sample.ip` will immediately crash the script with an `AttributeError`.
- [High] Event-type conditionals will always evaluate to `False` due to incorrectly extracting the event name via `str(sample.evsel)`.
- [High] The thread command name (`comm`) logic is broken and will silently fall back to `"Unknown"` for all events.
- [High] Instruction bytes containing NUL characters (`\x00`) will be silently truncated before disassembly.
- [High] Extracting the decoded instruction length relies on a hardcoded internal offset in `libxed.py`, leading to potential out-of-bounds array reads.
- [High] Missing length checks before `struct.unpack_from` leads to out-of-bounds reads and script termination on unvalidated payload buffers.
- [Medium] Untrusted payload data is used as a mathematical divisor without being checked for zero.
- [Medium] The exception handler designed to suppress `BrokenPipeError` traceback output contains a bug that triggers an uncatchable exception during interpreter shutdown.
--

commit 5f2caf09eb634efff61968a9fb75db89cffff164
Author: Ian Rogers <irogers@google.com>

perf intel-pt-events: Port intel-pt-events/libxed to use python module

This commit ports the intel-pt-events and libxed scripts to use a class
structure and a standalone python module. It removes Python 2 compatibility
and renames methods in libxed.py to snake case.

> diff --git a/tools/perf/python/intel-pt-events.py b/tools/perf/python/intel-pt-events.py
> new file mode 100755
> index 0000000000000..cb730d1009f45
> --- /dev/null
> +++ b/tools/perf/python/intel-pt-events.py

[ ... ]

> + def print_cbr(self, raw_buf: bytes) -> None:
> + """Print CBR data."""
> + data = struct.unpack_from("<BBBBII", raw_buf)
> + cbr = data[0]
> + f = (data[4] + 500) // 1000
> + p = ((cbr * 1000 // data[2]) + 5) // 10

Can struct.unpack_from() raise an exception if raw_buf is empty or smaller
than the requested format?

Is it possible for data[2] to be zero, triggering a ZeroDivisionError during
this calculation?

[ ... ]

> + def print_evt(self, raw_buf: bytes) -> None:

[ ... ]

> + data = struct.unpack_from("<BBH", raw_buf)
> + typ = data[0] & 0x1f
> + ip_flag = (data[0] & 0x80) >> 7
> + vector = data[1]
> + evd_cnt = data[2]

[ ... ]

> + pos = 4
> + for _ in range(evd_cnt):
> + data = struct.unpack_from("<QQ", raw_buf, pos)

Are there missing length checks on raw_buf? If evd_cnt is larger than the
remaining buffer size, wouldn't struct.unpack_from() raise a struct.error
and terminate the session?

[ ... ]

> + def disassem(self, insn: bytes, ip: int) -> tuple[int, str]:
> + """Disassemble instruction using LibXED."""
> + inst = self.disassembler.instruction()
> + self.disassembler.set_mode(inst, 0) # Assume 64-bit
> + buf = create_string_buffer(64)
> + buf.value = insn

Doesn't assigning insn to buf.value treat it as a null-terminated C string?
Since x86 machine code can contain \x00 bytes, won't this prematurely truncate
the instruction stream before disassembly? Would assigning to buf.raw be
safer?

[ ... ]

> + def print_common_ip(self, sample: perf.sample_event, symbol: str, dso: str) -> None:
> + """Print IP and symbol info."""
> + ip = sample.ip

Do these sample attributes exist on the new perf.sample_event C extension
object? Looking at the sample_member_def macro in tools/perf/util/python.c,
it seems the properties are exposed with a sample_ prefix, such as
sample_ip, sample_cpu, and sample_time. Will accessing sample.ip raise an
AttributeError?

[ ... ]

> + def do_process_event(self, sample: perf.sample_event) -> None:
> + """Process event and print info."""
> + comm = getattr(sample, "comm", "Unknown")
> + name = str(sample.evsel)

Does the perf.sample_event object expose a comm property? Without the legacy
environment manually injecting it into the event dictionary, will this
unconditionally fall back to Unknown? Would calling
session.process(sample.sample_pid).comm() be required to retrieve the
thread name?

Also, doesn't str(sample.evsel) return a formatted string like
evsel(instructions) rather than just instructions?

[ ... ]

> + if name.startswith("instructions"):

If name contains evsel(instructions), will this startswith() check, as well
as the exact matches like name == "ptwrite" below, always evaluate to false?

[ ... ]

> + def process_event(self, sample: perf.sample_event) -> None:
> + """Wrapper to handle interleaving and exceptions."""
> + try:
> + if self.args.interleave:
> + self.interleave_events(sample)
> + else:
> + self.do_process_event(sample)
> + except BrokenPipeError:
> + # Stop python printing broken pipe errors and traceback
> + with open(os.devnull, 'w', encoding='utf-8') as f:
> + sys.stdout = f
> + sys.exit(1)

When the with block ends, f is closed, but sys.stdout still points to it.
When sys.exit(1) triggers interpreter shutdown, will it attempt to flush the
closed sys.stdout and raise a secondary ValueError, defeating the purpose of
suppressing the traceback?

> diff --git a/tools/perf/python/libxed.py b/tools/perf/python/libxed.py
> new file mode 100755
> index 0000000000000..486987c6ec6de
> --- /dev/null
> +++ b/tools/perf/python/libxed.py

[ ... ]

> + def disassemble_one(self, inst, bytes_ptr, bytes_cnt, ip):
> + """Disassemble one instruction."""

[ ... ]

> + result = inst.buffer.value.decode('utf-8')
> + # Return instruction length and the disassembled instruction text
> + # For now, assume the length is in byte 166
> + return inst.xedd[166], result

Is it safe to rely on this hardcoded internal struct offset? If the layout of
xed_decoded_inst_t changes across libxed versions and byte 166 contains a
garbage value larger than bytes_cnt, will the calling loop in
print_common_ip() perform an out-of-bounds read and crash? Should this use
the official libxed API function to get the instruction length?

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