fix(rocprofiler-compute): Harden formula eval with AST node allowlist by xuchen-amd · Pull Request #9736 · ROCm/rocm-systems

xuchen-amd · 2026-08-05T20:32:36Z

Motivation

Add deny-by-default AST node validation to the metric formula evaluation pipeline. YAML config formula strings reach eval() through CodeTransformer without restriction on AST node types, allowing attribute chains ("".__class__.__mro__) and other constructs to bypass the existing function call allowlist. Users can supply arbitrary YAML configs via --config-dir.

Technical Details

Add ALLOWED_AST_NODES frozenset and a generic_visit override to CodeTransformer that rejects any AST node type not on the allowlist
Callers (build_eval_string, db_analysis.evaluate) catch ValueError from the transformer and report the offending expression via console_error
Suppress __builtins__ at both eval() sites by passing {"__builtins__": {}} as globals instead of {}

Issue Tracking

JIRA ID: ROCM-26855

Test Plan

Verify legitimate formulas (arithmetic, aggregation calls, ternaries) pass
Verify unsafe AST node types (Attribute, Lambda, ListComp, GeneratorExp, DictComp, SetComp) raise ValueError
Verify build_eval_string exits via console_error on unsafe input

Test Result

Verify legitimate formulas (arithmetic, aggregation calls, ternaries) pass
Verify unsafe AST node types (Attribute, Lambda, ListComp, GeneratorExp, DictComp, SetComp) raise ValueError
Verify build_eval_string exits via console_error on unsafe input

Submission Checklist

Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.

…ns suppression

…ests

abchoudh-amd

LGTM.
Could you add this is as Resolved Issues in Changelog?

vedithal-amd

Fix is sound. Verified the reported payload and variants are rejected, builtins are genuinely empty at both eval sites, and all 9,518 shipped analysis_configs/ formulas still pass the new transformer.

Also traced the parse=False branch at analysis_db.py:493 -- not a hole, those strings come from analysis_base.py:192 -> build_eval_string, so they're already validated. Worth a line in the PR body to save the next reader the trace.

Four things to fix before merge:

console_error aborts the run instead of warning and skipping the metric
the visit_Call allowlist is dead for all non-ammolite__ names
benign operators (ast.keyword, In/Is, bitwise) got caught in the narrowing
no test that builtins are suppressed at the eval sites

Rest are minor/nitpick, called out inline.

vedithal-amd · 2026-08-06T10:05:35Z

 from utils.utils_common import SUPPORTED_FIELD
 from utils.utils_counter_defs import SUPPORTED_DENOM

+ALLOWED_AST_NODES: frozenset[type] = frozenset({


This rejects ast.keyword (so ROUND(x, ndigits=0) breaks), In/NotIn/Is/IsNot, and all bitwise ops. None are escape vectors, so please add them back -- shipped configs don't hit these, but custom configs may.

Also worth one line above the frozenset on why Attribute is absent, since that's the whole security property:

# ast.Attribute is omitted deliberately: it is the dunder-chain escape.

vedithal-amd · 2026-08-06T10:05:35Z

+    ast.Call,
+    ast.Subscript,
+    ast.Slice,
+    ast.Index,


ast.parse hasn't emitted Index since 3.9 and pyproject.toml pins requires-python = ">=3.9". Dead entry.

vedithal-amd · 2026-08-06T10:05:35Z

 class CodeTransformer(ast.NodeTransformer):
    """Python AST visitor to transform user equation strings to df format."""

+    def generic_visit(self, node: ast.AST) -> ast.AST:


Bare ValueError could swallow an unrelated ValueError raised from inside the visitor and mislabel it as an invalid expression. Suggest an InvalidExpressionError(ValueError) and catching that at both call sites.

vedithal-amd · 2026-08-06T10:05:35Z

+            raise ValueError(f"Disallowed expression element: {type(node).__name__}")
+        return super().generic_visit(node)
+
    def visit_Call(self, node: ast.Call) -> ast.Call:


The SUPPORTED_CALL check never fires for non-ammolite__ names: generic_visit runs first and visit_Name has already turned the callee into a Subscript, so FOO(SQ_WAVES) becomes raw_pmc_df['FOO'](...) and SQ_WAVES[0]() passes too. Check before transforming:

def visit_Call(self, node: ast.Call) -> ast.Call: if not isinstance(node.func, ast.Name) or node.func.id not in SUPPORTED_CALL: raise InvalidExpressionError( f"Unsupported call: {astunparse.unparse(node.func).strip()}" ) self.generic_visit(node) node.func.id = SUPPORTED_CALL[node.func.id] return node

Verified against all 9,518 formulas in analysis_configs/: no regressions. This also folds the bare raise Exception on line 93 (and in visit_IfExp) into the same type, so the new except ValueError actually catches a typo'd formula instead of tracebacking.

vedithal-amd · 2026-08-06T10:05:35Z

-    transformer = CodeTransformer()
-    transformer.visit(ast_node)
+    try:
+        transformer = CodeTransformer()


Only .visit() can raise; move the constructor above the try. Same at analysis_db.py:592.

vedithal-amd · 2026-08-06T10:05:35Z

+        transformer = CodeTransformer()
+        transformer.visit(ast_node)
+    except ValueError as exc:
+        console_error(f"Invalid metric expression '{equation}': {exc}")


console_error exits, so one bad metric aborts the whole run. Every other failure in these two functions warns and degrades the cell (metric_evaluator.py:100-120 -> "N/A", analysis_db.py:541-543 -> None).

Suggest console_warning + return "" here and return None at analysis_db.py:595; evaluation_pipeline.py:191 already skips falsy cells and build_eval_string already returns "" for an empty equation. Rejecting the expression is the security property, terminating isn't.

Note this means test_build_eval_string_exits_on_unsafe_formula needs rewriting to assert the warning rather than SystemExit.

vedithal-amd · 2026-08-06T10:05:35Z

-            transformer = CodeTransformer()
-            transformer.visit(ast_node)
+            try:
+                transformer = CodeTransformer()


Same try/except as expression.py:189. Suggest a transform_expression(ast_node, source) helper in expression.py so the two error messages can't drift.

vedithal-amd · 2026-08-06T10:05:35Z

+                transformer = CodeTransformer()
+                transformer.visit(ast_node)
+            except ValueError as exc:
+                console_error(f"Invalid metric expression '{value}': {exc}")


value has already been through the $ -> sys_info["..."] rewrite at line 483, so the user sees mangled text instead of their YAML. build_eval_string gets this right by reporting the original equation.

vedithal-amd · 2026-08-06T10:05:35Z

                eval_result = eval(
                    compile(expr, "<string>", "eval"),
-                    {},
+                    {"__builtins__": {}},


Nothing asserts builtins are actually suppressed, so a revert to {} passes CI silently. One test would cover it:

def test_eval_expression_cannot_reach_builtins(self): """Builtins are suppressed at the eval site, so __import__ is unreachable.""" evaluator = MetricEvaluator(pd.DataFrame(), {}, {}) with patch("utils.metrics.metric_evaluator.console_warning") as mock_warning: assert evaluator.eval_expression("__import__('os')") == "N/A" assert mock_warning.called

vedithal-amd · 2026-08-06T10:18:22Z

Could you add this is as Resolved Issues in Changelog?

@abchoudh-amd and @xuchen-amd I dont think this is a user facing change right?

xuchen-amd added 2 commits August 5, 2026 16:19

fix(security): harden formula eval with AST node allowlist and builti…

b9da243

…ns suppression

test(security): add AST node allowlist and attack payload rejection t…

e2e4751

…ests

xuchen-amd requested a review from a team as a code owner August 5, 2026 20:32

github-actions Bot added the project: rocprofiler-compute label Aug 5, 2026

systems-assistant Bot added the organization: ROCm label Aug 5, 2026

abchoudh-amd approved these changes Aug 6, 2026

View reviewed changes

vedithal-amd requested changes Aug 6, 2026

View reviewed changes

Navigation Menu

Uh oh!

fix(rocprofiler-compute): Harden formula eval with AST node allowlist - #9736

fix(rocprofiler-compute): Harden formula eval with AST node allowlist#9736
xuchen-amd wants to merge 2 commits into
rocprofiler-compute-developfrom
users/xuchen-amd/harden_formula_eval

xuchen-amd commented Aug 5, 2026

Uh oh!

abchoudh-amd left a comment

Uh oh!

vedithal-amd left a comment

Uh oh!

vedithal-amd Aug 6, 2026

Uh oh!

vedithal-amd Aug 6, 2026

Uh oh!

vedithal-amd Aug 6, 2026

Uh oh!

vedithal-amd Aug 6, 2026

Uh oh!

vedithal-amd Aug 6, 2026

Uh oh!

vedithal-amd Aug 6, 2026

Uh oh!

vedithal-amd Aug 6, 2026

Uh oh!

vedithal-amd Aug 6, 2026

Uh oh!

vedithal-amd Aug 6, 2026

Uh oh!

vedithal-amd commented Aug 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

xuchen-amd commented Aug 5, 2026

Motivation

Technical Details

Issue Tracking

Test Plan

Test Result

Submission Checklist

Uh oh!

abchoudh-amd left a comment

Choose a reason for hiding this comment

Uh oh!

vedithal-amd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vedithal-amd commented Aug 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants