Re: [PATCH v1 35/58] perf event_analyzing_sample: Port event_analyzing_sample to use python module

sashiko-bot
2026-04-20T00:35:27Z

Sashiko AI review found 3 potential issue(s):
- [Critical] The script uses a hardcoded, predictable file path in a shared, world-writable directory (`/dev/shm/perf.db`), exposing a symlink attack vulnerability.
- [Medium] Event names in the database are wrapped in 'evsel(...)', altering the expected output and breaking compatibility.
- [Medium] Thread command name (comm) resolution fails or returns incorrect values for sub-threads.
--

commit 3f169dabd50cb00496ff1c84ac713efcfbc9dee4
Author: Ian Rogers <irogers@google.com>

perf event_analyzing_sample: Port event_analyzing_sample to use python module

This commit adds a port of the event_analyzing_sample script to use the
perf python module directly. It aims to improve performance by avoiding
the creation of intermediate dictionaries for all event fields.

> diff --git a/tools/perf/python/event_analyzing_sample.py b/tools/perf/python/event_analyzing_sample.py
> new file mode 100755
> index 0000000000000..f437ea25c0db5
> --- /dev/null
> +++ b/tools/perf/python/event_analyzing_sample.py
> @@ -0,0 +1,291 @@

[ ... ]

> +session: Any = None
> +
> +# Database connection
> +# If the perf.data has a big number of samples, then the insert operation
> +# will be very time consuming (about 10+ minutes for 10000 samples) if the
> +# .db database is on disk. Move the .db file to RAM based FS to speedup
> +# the handling, which will cut the time down to several seconds.
> +DB_PATH = "/dev/shm/perf.db" if os.path.exists("/dev/shm") else "perf.db"
> +con = sqlite3.connect(DB_PATH)

This was retained from the original script, but does this create a symlink
overwrite regression?

Since /dev/shm is world-writable, an unprivileged user could pre-create a
symlink at /dev/shm/perf.db pointing to an arbitrary system file. If a
privileged user runs this script, would sqlite3.connect() follow the symlink
and overwrite the target file?

[ ... ]

> +def process_event(sample: perf.sample_event) -> None:
> + """Callback for processing events."""
> + # Create and insert event object to a database so that user could
> + # do more analysis with simple database commands.
> +
> + # Resolve comm, symbol, dso
> + comm = "Unknown_comm"
> + try:
> + if session is not None:
> + proc = session.process(sample.sample_pid)

Does this code successfully resolve the command name for sub-threads?

Looking at the C extension's implementation, pyrf_session__process passes
the single sample_pid value to machine__find_thread() for both the pid and
tid arguments. This means it only finds the thread object if the TID equals
the PID.

Will this cause events originating from sub-threads to record the main
thread's comm or fail to resolve entirely, whereas the original callback
behavior resolved the specific sub-thread correctly?

> + if proc:
> + comm = proc.comm()
> + except TypeError:
> + pass
> +
> + # Symbol and dso info are not always resolved
> + dso = sample.dso if hasattr(sample, 'dso') and sample.dso else "Unknown_dso"
> + symbol = sample.symbol if hasattr(sample, 'symbol') and sample.symbol else "Unknown_symbol"
> + name = str(sample.evsel)

Does calling str() on the evsel object introduce a regression in how event
names are stored?

The python type implementation for evsel formats the string representation
as evsel(name). Unlike the original script which used the raw event name
like cycles, this code will record the string as evsel(cycles) in the
database's name column.

Will this implicit behavioral change break compatibility for downstream
database queries expecting the raw event name?

> +
> + # Create the event object and insert it to the right table in database
> + try:
> + event = create_event(name, comm, dso, symbol, sample.raw_buf)

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