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

sashiko-bot
2026-04-23T04:19:32Z

Sashiko AI review found 6 potential issue(s):
- [High] PostgreSQL COPY protocol violation due to missing binary trailer, resulting in data loss.
- [High] Silently masked PostgreSQL COPY failures.
- [High] Duplicate primary key generation for the `comm_threads` table.
- [Medium] Temporary directory leak on database initialization failure.
- [Medium] Incomplete implementation of `branch_types` table export.
- [Medium] Unsafe interpolation of database name leads to SQL and connection string injection.
--

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

perf export-to-postgresql: Port export-to-postgresql to use python module

This commit ports the export-to-postgresql script to use the perf Python
module API and libpq via ctypes, removing the PySide2 dependency. It keeps
the high performance binary file generation but implements lazy population
of lookup tables.

> diff --git a/tools/perf/python/export-to-postgresql.py b/tools/perf/python/export-to-postgresql.py
> new file mode 100755
> index 0000000000000..d1fa87e1d2b1a
> --- /dev/null
> +++ b/tools/perf/python/export-to-postgresql.py
> @@ -0,0 +1,697 @@
[ ... ]
> + def connect(self, db_to_use: str) -> None:
> + """Connect to database."""
> + conn_str = toclientstr(f"dbname = {db_to_use}")
> + self.conn = PQconnectdb(conn_str)

Is it possible for a specially crafted database name to inject arbitrary
libpq connection parameters here?

[ ... ]
> + def setup_db(self) -> None:
> + """Create database and tables. MUST be called after init."""
> + os.mkdir(self.output_dir_name)
> +
> + self.connect('postgres')
> + try:
> + self.do_query(f'CREATE DATABASE "{self.dbname}"')

Could a crafted -o argument break out of these double quotes to execute
arbitrary SQL against the default database?

[ ... ]
> + self.files['symbol'] = self.open_output_file("symbol_table.bin")
> + self.files['branch_type'] = self.open_output_file("branch_type_table.bin")
> + self.files['sample'] = self.open_output_file("sample_table.bin")
> + self.files['call_path'] = self.open_output_file("call_path_table.bin")
> +
> + self.write_evsel(0, "unknown")
> + self.write_machine(0, 0, "unknown")
> + self.write_thread(0, 0, 0, -1, -1)
> + self.write_comm(0, "unknown", 0, 0, 0)
> + self.write_dso(0, 0, "unknown", "unknown", "")
> + self.write_symbol(0, 0, 0, 0, 0, "unknown")
> + self.write_call_path(0, 0, 0, 0)

Is the branch_type_table.bin file ever populated?

It appears there is no write_branch_type method implemented, which means
the branch_types table will remain completely empty.

[ ... ]
> + def copy_output_file(self, f, table_name: str):
> + """Copy intermediate file to database."""
[ ... ]
> + ret = PQputCopyEnd(self.conn, None)
> + if ret != 1:
> + raise RuntimeError(f"PQputCopyEnd failed for {table_name}")
> +
> + res = PQgetResult(self.conn)
> + while res:
> + PQclear(res)
> + res = PQgetResult(self.conn)

Does this loop silently mask PostgreSQL server-side insertion failures?

If a COPY command fails on the server due to type mismatches or constraint
violations, PQresultStatus(res) would contain an error. Since the result
status is not checked before clearing, the script could leave the tables
empty while reporting a successful export.

[ ... ]
> + def get_comm_id(self, comm: str, thread_id: int) -> int:
> + """Get or create comm ID."""
> + if comm in self.caches['comms']:
> + comm_id = self.caches['comms'][comm]
> + else:
> + comm_id = self.next_id['comm']
> + self.write_comm(comm_id, comm, thread_id, 0, 0)
> + self.caches['comms'][comm] = comm_id
> + self.next_id['comm'] += 1
> +
> + key = (comm_id, thread_id)
> + if 'comm_threads' not in self.caches:
> + self.caches['comm_threads'] = {}
> + if key not in self.caches['comm_threads']:
> + self.write_comm_thread(comm_id, comm_id, thread_id)

Does this use comm_id as the primary key for the comm_threads table?

If multiple threads share the same comm string, get_comm_id will pass the
same comm_id as the comm_thread_id parameter to write_comm_thread(). This
would result in multiple rows with identical IDs, causing a duplicate key
constraint violation when finalize() adds the primary key.

[ ... ]
> + def finalize(self) -> None:
> + """Copy files to database and add keys/views."""
> + print("Copying to database...")
> + for name, f in self.files.items():
> + table_name = name + "s" if name != "call_path" else "call_paths"
> + if name == "evsel":
> + table_name = "selected_events"
> + self.copy_output_file(f, table_name)
> + self.close_output_file(f)

Could this cause a PostgreSQL COPY protocol violation resulting in data loss?

copy_output_file() reads the file up to EOF and streams it to the server.
However, close_output_file() appends the mandatory binary format trailer
(\377\377) to the file.

Since the trailer is written after the file has already been streamed, the
server will never receive it, which causes it to abort the COPY operation
and discard the inserted data.

[ ... ]
> +if __name__ == "__main__":
> + ap = argparse.ArgumentParser(
> + description="Export perf data to a postgresql database")
> + ap.add_argument("-i", "--input", default="perf.data",
> + help="Input file name")
> + ap.add_argument("-o", "--output", required=True,
> + help="Output database name")
> + args = ap.parse_args()
> +
> + exporter = PostgresExporter(args.output)
> + exporter.setup_db()
> +
> + session = None
> + error_occurred = False
> + try:
> + session = perf.session(perf.data(args.input),

If setup_db() raises an exception, will it leak the temporary data directory?

setup_db() creates the directory, but because it is called before the
try...finally block, an early failure (such as a database connection error)
will bypass the cleanup logic in the finally block and leave the directory
on the filesystem.

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