Re: [PATCH v2 36/58] perf export-to-sqlite: Port export-to-sqlite to use python module

[sashiko-bot@kernel.org]

Sashiko AI review found 7 potential issue(s):
- [High] Silent data loss due to incorrect Python API attribute access for sample metrics.
- [High] Complete loss of branch tracing data, severely breaking Intel PT export capabilities.
- [High] Inverted construction of the call path tree during callchain parsing.
- [High] Command-to-thread mappings are silently dropped for threads sharing command names.
- [High] Callchain processing poisons the symbol cache with zeroed boundaries.
- [High] DSO caching by short name aliases distinct binaries, corrupting long names and build IDs.
- [High] TOCTOU race condition in database file creation allows arbitrary file overwrite.
--

commit 51a807dd8fd1bd35b3c7aa76d6a4147ba7d86cc5
Author: Ian Rogers <irogers@google.com>
Subject: perf export-to-sqlite: Port export-to-sqlite to use python module

This commit ports the export-to-sqlite.py script to use the perf Python
module API and the standard library sqlite3 module. It implements lazy
population of lookup tables and adds callchain support.

> diff --git a/tools/perf/python/export-to-sqlite.py b/tools/perf/python/export-to-sqlite.py
> new file mode 100755
> index 0000000000000..a662b4f22cdb9
> --- /dev/null
> +++ b/tools/perf/python/export-to-sqlite.py

[ ... ]

> +    def get_comm_id(self, comm: str, thread_id: int) -> int:
> +        """Get or create comm ID."""
> +        if comm in self.caches['comms']:
> +            return self.caches['comms'][comm]

Is it intentional that the cache key for comms only uses the command string,
ignoring the thread_id?

If multiple threads share the same command name, the second thread will hit
the cache and receive the first thread's comm_id. This prevents insertion
into the comm_threads table for the second thread, dropping the association.

> +        comm_id = self.next_id['comm']
> +        self.con.execute("INSERT INTO comms VALUES (?, ?, ?, ?, ?)",
> +                         (comm_id, comm, thread_id, 0, 0))
> +        self.con.execute("INSERT INTO comm_threads VALUES (?, ?, ?)",
> +                         (comm_id, comm_id, thread_id))
> +        self.caches['comms'][comm] = comm_id
> +        self.next_id['comm'] += 1
> +        return comm_id
> +
> +    def get_dso_id(self, short_name: str, long_name: str,
> +                   build_id: str) -> int:
> +        """Get or create DSO ID."""
> +        if short_name in self.caches['dsos']:
> +            return self.caches['dsos'][short_name]

Does caching only by short_name cause aliasing if distinct shared objects
share the same short name?

If two distinct objects share a short name (e.g., libc.so.6), the first DSO
encountered will dictate the long_name and build_id for all subsequent
queries. Should the cache key include long_name or build_id to differentiate
them?

> +        dso_id = self.next_id['dso']
> +        self.con.execute("INSERT INTO dsos VALUES (?, ?, ?, ?, ?)",
> +                         (dso_id, 0, short_name, long_name, build_id))
> +        self.caches['dsos'][short_name] = dso_id
> +        self.next_id['dso'] += 1
> +        return dso_id
> +
> +    def get_symbol_id(self, dso_id: int, name: str, start: int,
> +                      end: int) -> int:
> +        """Get or create symbol ID."""
> +        key = (dso_id, name)
> +        if key in self.caches['symbols']:
> +            return self.caches['symbols'][key]

Will this cache key lead to missing symbol boundaries?

When processing callchains below, this is called with start and end as 0. If
the same symbol is later encountered as a primary sample target with real
boundaries, the cache hit will prevent the real boundaries from being stored.

> +        symbol_id = self.next_id['symbol']
> +        self.con.execute("INSERT INTO symbols VALUES (?, ?, ?, ?, ?, ?)",
> +                         (symbol_id, dso_id, start, end, 0, name))
> +        self.caches['symbols'][key] = symbol_id
> +        self.next_id['symbol'] += 1
> +        return symbol_id

[ ... ]

> +        # Handle callchain
> +        call_path_id = 0
> +        if hasattr(sample, 'callchain') and sample.callchain:
> +            parent_id = 0
> +            for node in sample.callchain:

Does this loop construct the call tree inverted?

The perf Python module yields callchain frames from the innermost frame
(current instruction) outward. Because parent_id is assigned sequentially, the
innermost frame becomes the root, and the outermost frame becomes a deep leaf.

> +                node_dso = getattr(node, 'dso', None) or getattr(node, 'map', None)
> +                node_symbol = getattr(node, 'symbol', None) or getattr(node, 'sym', None)
> +
> +                dso_name = "Unknown_dso"
> +                if node_dso:
> +                    dso_name = getattr(node_dso, 'name', "Unknown_dso") or "Unknown_dso"
> +
> +                symbol_name = "Unknown_symbol"
> +                if node_symbol:
> +                    symbol_name = getattr(node_symbol, 'name', "Unknown_symbol") or "Unknown_symbol"
> +
> +                node_dso_id = self.get_dso_id(dso_name, dso_name, "")
> +                node_symbol_id = self.get_symbol_id(node_dso_id, symbol_name, 0, 0)
> +
> +                parent_id = self.get_call_path_id(parent_id, node_symbol_id, node.ip)
> +            call_path_id = parent_id
> +        else:
> +            call_path_id = 0
> +
> +        # Insert sample
> +        self.con.execute("""
> +            INSERT INTO samples VALUES (
> +                NULL, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?
> +            )
> +        """, (
> +            self.get_event_id(getattr(sample.evsel, 'name', str(sample.evsel))),
> +            0, thread_id, comm_id,
> +            dso_id, symbol_id, getattr(sample, 'sym_offset', 0),
> +            sample.sample_ip, sample.sample_time, sample.sample_cpu,
> +            0, 0, 0, 0,  # to_dso, to_symbol, to_sym_offset, to_ip

Does this drop branch tracking information needed for Intel PT analysis?

The branch destination columns and branch_type are hardcoded to 0 instead of
extracting the data from sample.brstack.

> +            getattr(sample, 'sample_period', 0) or 0,
> +            getattr(sample, 'sample_weight', 0) or 0,
> +            getattr(sample, 'transaction_', 0),
> +            getattr(sample, 'data_src', 0),
> +            0,  # branch_type
> +            getattr(sample, 'in_tx', 0),
> +            call_path_id,
> +            getattr(sample, 'insn_count', 0),
> +            getattr(sample, 'cyc_count', 0),
> +            getattr(sample, 'flags', 0)

Will these getattr calls silently return 0 and cause data loss for the sample
metrics?

The perf Python module exposes the metrics as sample_data_src,
sample_insn_cnt, and sample_cyc_cnt, rather than data_src, insn_count, and
cyc_count. Additionally, transaction_, in_tx, and flags do not appear to be
exposed by the Python API at all.

> +        ))

[ ... ]

> +if __name__ == "__main__":
> +    ap = argparse.ArgumentParser(
> +        description="Export perf data to a sqlite3 database")
> +    ap.add_argument("-i", "--input", default="perf.data",
> +                    help="Input file name")
> +    ap.add_argument("-o", "--output", default="perf.db",
> +                    help="Output database name")
> +    args = ap.parse_args()
> +
> +    try:
> +        fd = os.open(args.output, os.O_CREAT | os.O_EXCL | os.O_WRONLY)
> +        os.close(fd)
> +    except FileExistsError:
> +        print(f"Error: {args.output} already exists")
> +        sys.exit(1)
> +
> +    exporter = DatabaseExporter(args.output)

Can an attacker exploit a time-of-check to time-of-use race here if the
database is created in a shared directory like /tmp?

The script securely creates the file with O_CREAT | O_EXCL to prevent
overwriting existing files, but immediately closes the file descriptor.

DatabaseExporter then calls sqlite3.connect(), which resolves the path again
by name. Between the close and the connect, the file could be deleted and
replaced with a symlink to a sensitive file, defeating the O_EXCL protection.

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