Skip to content

Bound a sheet extent no cell substantiates - #177

Open
SimonCropp wants to merge 1 commit into
mainfrom
bound-declared-sheet-extent
Open

Bound a sheet extent no cell substantiates#177
SimonCropp wants to merge 1 commit into
mainfrom
bound-declared-sheet-extent

Conversation

@SimonCropp

Copy link
Copy Markdown
Member

#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, which SheetGridBuilder then materialises a cell at a time.

Two paths carried such a range through ResolveRange unclipped:

  • a sheet with no cells at all, where dimension is the only statement of extent and there is nothing to intersect it against
  • a print area overlapping none of the cells, which the used range therefore never clips — clipped.IsEmpty ? area : clipped handed back the raw declared area

Measured on the second, against a ~1.6KB crafted workbook:

declared rows before after
100 2.0s
400 7.5s
1,600 52.7s, 25GB allocated, 15.9GB peak working set
1,048,576 (full grid) OOM 310ms

And the dimension path, 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 UsedRange and 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 ParseWorkbook skips 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 against SpreadsheetParser.MaxDeclaredCells rather than a hardcoded row count.

The Workbook test builder gained a printArea overload; 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

  • ExtendForOverflow can still widen a real sheet to 1,000 columns via maxShapedColumn. It needs real cells, so it is bounded by the file, and that constant was a deliberate call.
  • A legitimately enormous populated sheet will still exhaust memory. That is a truncation policy decision about real content, not a crafted-input hole.

Follows #175. Turned up while reviewing #176.

#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.
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.

1 participant