Re: [PATCH v1 30/58] perf flamegraph: Port flamegraph to use python module

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

Sashiko AI review found 9 potential issue(s):
- [Medium] Python 3.9+ type hinting syntax `list[Node]` and `list[Dict]` raises TypeError on older Python versions.
- [Medium] The `--allow-download` flag logic is broken and inadvertently prevents downloading the template.
- [Medium] Faulty handling of custom HTTP URLs passed to `--template`.
- [Medium] Informational minimal HTML fallback message is printed to stdout, corrupting data streams.
- [Medium] Unsanitized JSON data injected into HTML ` ` tag allows Local HTML Injection / XSS.
- [Medium] Missing fallback logic for samples without callchains.
- [Medium] Terminal escape sequence injection during MD5 checksum failure prompt.
- [Medium] Algorithmic complexity bottleneck in `find_or_create_node` causes O(N^2) performance degradation.
- [Low] Weak cryptographic hash algorithm (MD5) used for template integrity verification.
--

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

perf flamegraph: Port flamegraph to use python module

Add a port of the flamegraph script that uses the perf python module
directly. This approach is significantly faster than using perf script
callbacks as it avoids creating intermediate dictionaries for all
event fields.

> diff --git a/tools/perf/python/flamegraph.py b/tools/perf/python/flamegraph.py
> new file mode 100755
> index 0000000000000..090e07a0992d5
> --- /dev/null
> +++ b/tools/perf/python/flamegraph.py
[ ... ]
> +class Node:
> + """A node in the flame graph tree."""
> + def __init__(self, name: str, libtype: str):
> + self.name = name
> + self.libtype = libtype
> + self.value: int = 0
> + self.children: list[Node] = []

Will this script raise a TypeError on older Python versions? Using
list[Node] for type hinting relies on PEP 585, which requires Python 3.9+.
Could this use typing.List instead to maintain compatibility with older
environments?

> +
> + def to_json(self) -> Dict[str, Union[str, int, list[Dict]]]:

Similarly, does using list[Dict] here cause compatibility issues on versions
prior to Python 3.9?

[ ... ]
> + @staticmethod
> + def find_or_create_node(node: Node, name: str, libtype: str) -> Node:
> + """Find a child node with the given name or create a new one."""
> + for child in node.children:
> + if child.name == name:
> + return child
> + child = Node(name, libtype)
> + node.children.append(child)
> + return child

Could this linear search loop cause severe performance degradation? For
exceptionally wide call graphs with many unique symbols at the same depth,
iterating over node.children for every frame could create an O(N^2)
bottleneck.

[ ... ]
> + def process_event(self, sample) -> None:
[ ... ]
> + callchain = sample.callchain
> + if callchain:
> + # We want to traverse from root to leaf.
> + # perf callchain iterator gives leaf to root.
> + # We collect them and reverse.
> + frames = list(callchain)
> + for entry in reversed(frames):
> + name = entry.symbol or "[unknown]"
> + libtype = self.get_libtype_from_dso(entry.dso)
> + node = self.find_or_create_node(node, name, libtype)
> + node.value += 1

Does this skip adding sample details if the profile was recorded without
callchains?

If sample.callchain is empty, the script skips the frame processing loop
entirely and only increments the root process node, which might result in
flamegraphs that only display process names. Could it fall back to the
instruction pointer or symbol?

[ ... ]
> + def run(self) -> None:
[ ... ]
> + template = self.args.template
> + template_md5sum = None
> + output_str = None
> + if not os.path.isfile(template):
> + if not self.args.allow_download:
> + print(f"Warning: Flame Graph template '{template}' does not exist.",
> + file=sys.stderr)
> + template = (
> + "https://cdn.jsdelivr.net/npm/d3-flame-graph@4.1.3/dist/templates/"
> + "d3-flamegraph-base.html"
> + )

Does this logic inadvertently prevent the template from downloading when
--allow-download is passed?

If self.args.allow_download is true, the script skips updating the
template variable to the CDN URL, keeping it as the non-existent local path.
This causes the subsequent open() to fail and fall back to minimal HTML.

Also, can this overwrite a custom HTTP URL provided via the --template
argument?

Since os.path.isfile() evaluates to false for HTTP URLs, providing
a custom URL without --allow-download causes the script to warn that the
template does not exist and silently overwrite the user's URL with the
default cdn.jsdelivr.net URL.

[ ... ]
> + if use_minimal:
> + print("Using internal minimal HTML that refers to d3's web site. JavaScript " +
> + "loaded this way from a local file is typically blocked unless your " +
> + "browser has relaxed permissions.")
> + output_str = MINIMAL_HTML

Will this print statement corrupt the output stream if the user specifies
-o - to output the data to stdout? Writing informational warnings to standard
output instead of standard error might cause downstream tools or browsers to
fail when parsing the resulting HTML or JSON.

> +
> + elif template_md5sum:
> + download_md5sum = hashlib.md5(output_str.encode("utf-8")).hexdigest()

Is MD5 robust enough for template integrity verification? Since MD5 is
vulnerable to collision attacks, might it allow a maliciously modified
template to produce the expected hash?

> + if download_md5sum != template_md5sum:
> + s = None
> + while s not in ["y", "n"]:
> + s = input(f"""Unexpected template md5sum.
> +{download_md5sum} != {template_md5sum}, for:
> +{output_str}
> +continue?[yn] """).lower()

Can printing the unverified output_str directly to the terminal inject
malicious ANSI escape sequences? If the downloaded template from an untrusted
source fails the checksum, embedding the raw payload into the input() prompt
could manipulate the user's terminal emulator.

> + if s == "n":
> + sys.exit(1)
> +
> + output_str = output_str.replace("/** @options_json **/", options_json)
> + output_str = output_str.replace("/** @flamegraph_json **/", stacks_json)

Does embedding the JSON directly into the HTML script block allow for
cross-site scripting or HTML injection?

Since json.dumps() does not escape HTML special characters like < or /,
could a maliciously crafted process or thread name prematurely terminate the
script block and execute arbitrary JavaScript?

> + output_fn = self.args.output or "flamegraph.html"

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