Bound a sheet extent no cell substantiates - #177
Open
SimonCropp wants to merge 1 commit into
Open
Conversation
#175 stopped a crafted row or column INDEX overflowing the grid loop counter, but not the EXTENT it counts to. dimension and the print-area defined name are strings, and nothing ties either to the size of the package: both stay inside Excel's 16,384 x 1,048,576 grid while naming all 17 billion cells of it. Two paths carried such a range to SheetGridBuilder unclipped — a sheet with no cells at all, where dimension is the only statement of extent, and a print area overlapping none of the cells, which the used range therefore never clips. Measured on the second: a 1.6KB workbook declaring 1,600 rows allocated 25GB and took 52s, and the full grid it can declare is 655x that again. ResolveRange now caps a range nothing substantiates at 100,000 cells, clamping the row axis. A real sheet's extent comes from the cells it carries and is bounded by the file, so it is never capped — the corpus's largest real sheet is 43,736 cells, and no workbook in it has either shape.
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.
#175 stopped a crafted row or column index overflowing the grid loop counter, but not the extent it counts to.
dimensionand the print-area defined name are strings, and nothing ties either to the size of the package: both stay inside Excel's 16,384 x 1,048,576 grid while naming all 17 billion cells of it, whichSheetGridBuilderthen materialises a cell at a time.Two paths carried such a range through
ResolveRangeunclipped:dimensionis the only statement of extent and there is nothing to intersect it againstclipped.IsEmpty ? area : clippedhanded back the raw declared areaMeasured on the second, against a ~1.6KB crafted workbook:
And the
dimensionpath, same shape: 400 rows took 14.3s before, the full grid now parses in 143ms.The rule
A range that no cell substantiates is capped at 100,000 cells, clamping the row axis — a range is at most 16,384 columns wide, so rows alone always bring the product under.
A real sheet's extent comes from
UsedRangeand is bounded by the file, so it is never capped. That is what lets the cap be this small: the corpus's largest real sheet is 43,736 cells, and no workbook in it has either of the two shapes above.The print area is still honoured as declared rather than dropped — returning null would skip the sheet, and
ParseWorkbookskips its drawings along with it, which would lose a picture-only sheet.Tests
Four added to
GridLimitTests, since this is the same denial of service as #174 by a second route. Two assert the cap holds against a full-grid declaration on each path; two are the controls that it does not over-narrow — an extent under the cap is kept whole, and a print area the cells do substantiate still clips to them. They assert againstSpreadsheetParser.MaxDeclaredCellsrather than a hardcoded row count.The
Workbooktest builder gained aprintAreaoverload; the print area is a workbook-level defined name, so there was no way to author one before.Full suite in the container: 3,713 passed, no baseline drift.
Not in scope
ExtendForOverflowcan still widen a real sheet to 1,000 columns viamaxShapedColumn. It needs real cells, so it is bounded by the file, and that constant was a deliberate call.Follows #175. Turned up while reviewing #176.