fix(rocprofiler-compute): restrict metric expression evaluation#8918
Open
fjankovi wants to merge 7 commits into
Open
fix(rocprofiler-compute): restrict metric expression evaluation#8918fjankovi wants to merge 7 commits into
fjankovi wants to merge 7 commits into
Conversation
Replace Python eval calls in metric evaluation with a restricted AST interpreter that only permits supported arithmetic, data lookups, aggregation helpers, and Series.where. Reject imports, arbitrary calls and attributes, dynamic subscripts, and overly complex expressions. Add regression coverage for code-execution payloads in both analyzer paths.
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens rocprofiler-compute’s metric/expression evaluation by replacing Python eval() with a restricted AST-based interpreter, preventing workspace-supplied expressions from executing arbitrary code in the analyst’s environment.
Changes:
- Introduces
utils.metrics.safe_expression.evaluate_expression()as a restricted evaluator for metric expressions. - Replaces
eval()usage in both the YAML metric evaluator and analysis DB evaluator with the restricted evaluator. - Adds regression tests ensuring code-execution payloads and disallowed Python constructs are rejected.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/rocprofiler-compute/tests/test_metric_utils.py | Adds unit tests for the restricted expression evaluator and updates MetricEvaluator tests to patch evaluate_expression. |
| projects/rocprofiler-compute/tests/test_analysis_db.py | Adds regression test asserting analysis DB expression evaluation can’t execute Python payloads. |
| projects/rocprofiler-compute/src/utils/metrics/safe_expression.py | Adds the new restricted AST evaluator implementation. |
| projects/rocprofiler-compute/src/utils/metrics/metric_evaluator.py | Switches metric evaluation from eval() to evaluate_expression with a function whitelist. |
| projects/rocprofiler-compute/src/utils/metrics/evaluation_pipeline.py | Removes obsolete commented-out eval() experimentation. |
| projects/rocprofiler-compute/src/rocprof_compute_analyze/analysis_db.py | Switches analysis DB expression evaluation from eval() to evaluate_expression. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
🎉 All checks passed! This PR is ready for review. |
Limit metric expressions to the condition and optional other arguments supported by the safe expression language. Reject extra positional arguments before evaluation and cover the mutation case with a regression test.
Translate RecursionError from parsing or recursive AST evaluation into UnsafeExpressionError so analyzer callers continue to return N/A or None. Add regression coverage for expressions below the node limit that exceed Python's recursion depth.
Set a known recursion limit only around deep-expression evaluation and restore the process setting afterward. Count AST nodes before lowering the limit so the test remains deterministic across runner configurations.
fjankovi
marked this pull request as draft
July 21, 2026 15:17
Disallow binary arithmetic on scalar strings and string-containing Series to prevent workspace expressions from amplifying small inputs into excessive allocations. Preserve numeric object-Series arithmetic and cover scalar, NumPy-integer, concatenation, and Series cases.
Reject non-scalar conditional tests before truth-value conversion and translate ambiguous scalar values into UnsafeExpressionError. Preserve scalar Python truthiness and add coverage for Series, arrays, containers, pd.NA, and valid scalar conditions.
fjankovi
marked this pull request as ready for review
July 21, 2026 16:00
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Metric expressions loaded from profiling workspaces were evaluated using
Python
eval(). Supplying an empty globals dictionary still exposed Pythonbuiltins, allowing a crafted workspace configuration to execute arbitrary
code in the analyst's account.
Technical Details
approved dataframe and system-information lookups, whitelisted metric
helper functions, and
pandas.Series.where.dynamic subscripts, callable runtime values, and overly complex expressions.
eval()in both the standard metric evaluator and analysis databaseevaluator.
language features.
Issue Tracking
JIRA ID: SWSPLAT-24278
Test Plan
eval()orexec()calls.Test Result
eval()orexec()calls foundSubmission Checklist