Prevent unbounded Split on chart number formats (use IndexOf to select signed section)#1925
Merged
Merged
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75086225ae
ℹ️ 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".
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1925 +/- ##
==========================================
+ Coverage 73.29% 73.33% +0.04%
==========================================
Files 2256 2521 +265
Lines 262814 283196 +20382
Branches 56792 60589 +3797
==========================================
+ Hits 192633 207689 +15056
- Misses 44317 47871 +3554
- Partials 25864 27636 +1772 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
string.Split(';')on attacker-controlled chart number format strings, which can allocate unbounded arrays/substrings and cause memory amplification or OOM during chart/pdf rendering.Description
numberFormat.Split(';')with boundedIndexOflookups inSelectSignedNumberFormatSection, selecting only the needed section for positive, negative, or zero values.TrimNumberFormatSectionhelper to trim whitespace from the selected substring without allocating intermediate splits.FlowDrawing_SelectsSignedNumberFormatSectionWithoutSplittingAllSeparatorstoOfficeIMO.Tests/Pdf/PdfDocumentChartDrawingTests.csthat verifies selection works for positive, negative, and zero with a semicolon-heavy format.Testing
dotnet build OfficeIMO.Drawing/OfficeIMO.Drawing.csproj --framework net8.0which succeeded.dotnet test OfficeIMO.Tests/OfficeIMO.Tests.csproj --framework net8.0 --filter FullyQualifiedName~PdfDocumentChartDrawingTests, but the full test run was blocked by toolchain constraints: the repo includes projects targetingnet10.0and one project uses a preview language feature (null conditional assignment) that the installed SDK configuration rejected, preventing the test run from completing.git diff --checkwas run to validate whitespace and formatting; no issues were reported.Codex Task