Skip to content

fix(compare): bound aggregate decode memory - #31

Merged
frathe merged 4 commits into
mainfrom
codex/fix-memory-exhaustion-vulnerability-in-compare-feature
Sep 16, 2026
Merged

frathe merged 4 commits into
mainfrom
codex/fix-memory-exhaustion-vulnerability-in-compare-feature

Conversation

@frathe

@frathe frathe commented Sep 16, 2026

Copy link
Copy Markdown
Owner

Problem and resulting behavior

Comparison previously decoded and retained both selected sources without a combined admission limit, including GIF frames it never displayed. Each pane now checks decoded-image admission against half the shared image-cache budget and returns a localized refusal when its allowance is exceeded.

  • Header-only estimates allow eight bytes per pixel for supported 16-bit formats. Cached comparisons check the actual retained bytes of a detached first-frame record; fresh decodes receive the same final check.
  • GIF comparison loads decode only the first displayed frame. Partial-animation records never replace complete animated cache records.
  • English and German budget-refusal messages are delivered at the UI boundary. The renamed and new comparison tests have current shard assignments.
  • Qodana's unused-function warning for IsAnimatedGIF is suppressed only on that declaration: the comparison loader calls it to protect canonical cache records.

The equal split and header estimate are deliberately conservative. This limits decoded-image admission; it does not claim to cap all transient decoder, renderer, or process memory.

Verification

  • Regression tests were seen failing before the fix for 16-bit cold/cache admission and the refusal toast, then passing afterward.
  • Focused comparison, byte-accounting, cache-record, preload, and GIF-fallback tests pass.
  • Complete native internal/imaging, internal/ui/display, and internal/ui/compare package tests pass, as do locale parity and English identity checks.
  • make verify-build passes (formatting, generated assets/notices, TUF root, Qodana exclusions, vet, build).
  • make check-test-shards passes in Linux/amd64 Docker: 687 runnable tests across three shards.
  • GoLand inspections found no new issues; existing duplicate test setup remains covered by the exact test-file Qodana exclusion.
  • Local make verify correctly refuses the ARM Docker daemon. PR CI supplies the full native Linux/amd64 race suite and platform guards; all eight jobs passed on 8cdbb18, and checks run again for the final follow-up commit.

Hosted code/security review dispositions and final-head CI/Qodana/CodeQL results are tracked in this PR. The separate GitHub Advanced Security AI scan currently fails before producing results because its requested model is unsupported; it is distinct from the Codex connector security review.

Evidence record: finished_refactorings/2026-09-16-bound-comparison-memory.md.


Codex Task

@github-actions

github-actions Bot commented Sep 16, 2026

Copy link
Copy Markdown

Qodana for Go

It seems all right 👌

No new problems were found according to the checks applied

💡 Qodana analysis was run in the pull request mode: only the changed files were checked
☁️ View the detailed Qodana report

Contact Qodana team

Contact us at qodana-support@jetbrains.com

@frathe
frathe marked this pull request as ready for review September 16, 2026 19:18
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 16, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review 🔄 Running since 2026-09-16T19:28:02.300851Z bd39147 Manual request
🔒 Security Review 🔄 Running since 2026-09-16T19:28:02.340180Z bd39147 Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

frathe commented Sep 16, 2026

Copy link
Copy Markdown
Owner Author

Review-loop disposition for bd39147:

  • Qodana's GoUnusedExportedFunction on imaging.IsAnimatedGIF is a false positive: internal/ui/compare.go calls it before caching comparison records. Commit bd39147 retains the function and adds a declaration-scoped suppression with its caller's purpose documented.
  • GoLand reports no findings for the declaration or caller; formatting and git diff --check pass. The previous head 8cdbb18 passed all eight CI jobs, all four Linux race partitions, and both CodeQL analyses. Its Codex code and security reviews completed without findings. Fresh CI/Qodana/CodeQL now run on bd39147.
  • The separate GitHub Advanced Security AI scan failed before producing results with CAPIError: 400 The requested model is not supported; this is distinct from the completed Codex connector security review.

@codex review

frathe commented Sep 16, 2026

Copy link
Copy Markdown
Owner Author

@codex security review

Please review the latest pushed commit bd39147 after the documented Qodana false-positive disposition. The prior code/security round on 8cdbb18 completed without findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 👍

Reviewed commit: bd39147d94

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@chatgpt-codex-connector

Copy link
Copy Markdown

🛡️ Codex Security Review · Automatically triggered

Security review completed. No security issues were found in this pull request.

Reviewed commit: bd39147d94

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

frathe commented Sep 16, 2026

Copy link
Copy Markdown
Owner Author

Final-head review evidence for bd39147:

  • Fresh Codex code review: no major issues; reviewed commit bd39147d94.
  • Fresh Codex security result: completed with no security issues; reviewed commit bd39147d94.
  • CI: all eight jobs passed, including validation, four Linux race partitions, Windows guards, and both macOS architecture guards.
  • CodeQL: both analyses passed; no new alerts in changed code.
  • Qodana: success. Inspected artifact 10465540382's actual post-suppression qodana.sarif.json; revisionId matches the full head and results is empty. The false-positive disposition is recorded in bd39147 and the preceding review-loop comment.
  • No review threads remain unresolved. CI tested merge abd8b2658d6f7f38255d8034a664b5360f26025b, comprising this head and base 459663d635fa533032e6af2330f672532e0634c7.

The Codex summary panel still displays Running at this snapshot despite the explicit code/security completion comments linked above. No duplicate review requests have been posted.

Separate limitation: GitHub Advanced Security's AI scan failed before analysis with CAPIError: 400 The requested model is not supported. That scan produced no verification result and is distinct from the completed Codex security review. No security policy or check was disabled.

No merge or release was performed.

@frathe
frathe merged commit d02a418 into main Sep 16, 2026
13 of 14 checks passed
@frathe
frathe deleted the codex/fix-memory-exhaustion-vulnerability-in-compare-feature branch September 16, 2026 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant