Skip to content

reject negative cost and call counts in callgrind parser#112

Open
nvxbug wants to merge 1 commit into
jrfonseca:mainfrom
nvxbug:callgrind-nonneg-costs
Open

reject negative cost and call counts in callgrind parser#112
nvxbug wants to merge 1 commit into
jrfonseca:mainfrom
nvxbug:callgrind-nonneg-costs

Conversation

@nvxbug

@nvxbug nvxbug commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

CallgrindParser.parse_cost_line reuses the subposition pattern for the cost columns, so a cost written as a relative or signed token slips through even though callgrind costs are non-negative integer event counts. A file with a line like 0 -90 feeds that negative value straight into the per-function and global SAMPLES totals, which deflates the denominator and pushes the other functions well past their real share of time (and silently drops the tampered function). parse_association_spec has the same gap, taking the calls= count through a bare int(), so calls=-5 lands as a negative call tally on the node and edge.

Validate that the cost columns and the call count are non-negative integers and treat a line that violates that as unrecognized, the same way the parser already handles any other malformed line. Keeping the check inside the parser means a crafted profile cannot skew the totals before the rest of the pipeline sees them, and it matches the non-negative form the cost regex and the existing _call_re already describe.

@nvxbug

nvxbug commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

any update?

@codecov

codecov Bot commented Jul 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.95%. Comparing base (0896778) to head (0b02e6c).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
gprof2dot.py 60.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #112      +/-   ##
==========================================
- Coverage   85.01%   84.95%   -0.07%     
==========================================
  Files           1        1              
  Lines        2329     2333       +4     
==========================================
+ Hits         1980     1982       +2     
- Misses        349      351       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jrfonseca

Copy link
Copy Markdown
Owner

Hmm. There are hundreds of different ways profile input data can be wrong and inconsistent. Each check might be small, but when taken all together the end result will be a barrage of error checks overwhelming the logic.

What's the end goal here?

If these are errors that can normally arise due to bugs in profile tools, I'm happy to add checks, so an user knows they need to update their tool. If the goal here is to protect against potential maliciously crafted input, then this is a non-goal. This tool should not be used with untrusted input data.

@nvxbug

nvxbug commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Fair question. This came from poking the parser with malformed input rather than any real tool's output, so by your framing it falls in the non-goal bucket. There's also a point against it I hadn't considered: cg_diff emits profiles with legitimately negative counts, so hard-rejecting them could break someone feeding a diff through gprof2dot. Happy to close this, or I can rework it to just warn on suspicious counts if you think that's worth having.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants