XSSF: validate row/column/cell index before mutating the sheet XML

[ken-swyfft]

The problem

XSSFSheet.CreateRow adds the row element to sheetData before the index is validated:

public virtual IRow CreateRow(int rownum)
{
    ...
    ctRow = worksheet.sheetData.AddNewRow();                   // XML mutated here
    ...
    XSSFRow r = new XSSFRow(ctRow, this) { RowNum = rownum };  // validated here, throws
    _rows[rownum] = r;                                         // never reached
    return r;
}

When RowNum rejects the index the element is already in the XML, and because _rows is only updated afterwards the row is orphaned — invisible to GetRow and PhysicalNumberOfRows, but still written to the file.

The result is a workbook Excel refuses to open. This is enough to produce one:

var sheet = workbook.CreateSheet("Input");
for (int r = 0; r < 3; r++)
{
    sheet.CreateRow(r).CreateCell(0).SetCellValue(r);
}

for (int i = 0; i < 3; i++)
{
    try { sheet.CreateRow(-1); } catch (ArgumentException) { /* probe, carry on */ }
}

xl/worksheets/sheet1.xml:

<sheetData>
  <row r="4"/><row r="4"/><row r="4"/>
  <row r="1"><c r="A1"><v>0</v></c></row>
  <row r="2"><c r="A2"><v>1</v></c></row>
  <row r="3"><c r="A3"><v>2</v></c></row>
</sheetData>

Every leaked row carries the same r — XSSFRow's constructor fills in LastRowNum + 2 for an unset r before the setter throws — so the rows are both duplicated and ahead of the real ones, breaking the required ascending order. Excel reports "We found a problem with some content" and strips the sheet's data when repairing it.

Two things make this hard to notice. NPOI re-reads the malformed file without complaint (PhysicalNumberOfRows comes back as 4), so a write-out/read-back round trip looks fine — only the raw part XML shows the damage. And the caller doesn't have to do anything unusual: catching the exception and carrying on is exactly what "does this optional cell exist?" probing code does. We hit this in production, where a workbook generator probed named cells that some templates lack; every failed probe added another phantom row, and the delivered spreadsheets opened as corrupt.

XSSFSheet.CreateColumn and XSSFRow.CreateCell have the same mutate-then-validate shape. CreateColumn leaves a col definition for a column that was never created. The orphaned cell doesn't reach the file today, but only because XSSFRow.OnDocumentWrite rebuilds the row's cell array when the counts disagree — the object model is still left inconsistent after the throw.

The change

Hoist the bounds checks so nothing is written until the index is known good.

This makes XSSF consistent with the rest of the library rather than introducing a new convention — SXSSFSheet.CreateRow, SXSSFRow.CreateCell, XSSFColumn.CreateCell and XSSFRow.GetCell all validate first, as does HSSFSheet.CreateRow. The row check is copied from SXSSFSheet.CreateRow, and XSSFRow.CreateCell now calls the existing XSSFCell.CheckBounds (widened from private to internal) instead of reaching it indirectly via SetCellNum after the element has been added.

Exception types and messages are unchanged, so the existing BaseTestRow.BaseTestRowBounds / BaseTestCellBounds assertions still hold — only the side effect of the throw is gone.

Tests

Four tests in TestXSSFSheet and TestXSSFRow:

• one per creation method, asserting the raw element count is unchanged after a rejected index (the object model can't see the leak, so the CT_ counts are what has to be checked);
• an end-to-end one that writes the workbook out and inspects the persisted part XML, asserting rows are unique and ascending and no phantom column definition appears. Reverting the source change fails it with Expected: "1,2,3" But was: "4,4,4,1,2,3" and dumps the offending XML.

Verified on master: the full NPOI.OOXML.TestCases suite passes (1854 tests, 0 failures) on net10.0, and the new tests pass on net472 as well.

---

Submitted by Claude (an AI assistant) on behalf of @ken-swyfft. The analysis, patch and tests were produced by Claude and reviewed before submission.

[ken-swyfft]

Two updates.

Pushed a follow-up commit tidying the new test helper — the package is now opened read-only, the MemoryStream is disposed, and RegexOptions.Compiled is dropped from a single-use regex. It also corrects a @throws RuntimeException doc comment to ArgumentException on XSSFCell.CheckBounds, the line this patch widens to internal (a leftover from the Java original).

The red CI here is unrelated to this change — restore fails before compile on the System.Security.Cryptography.Xml 10.0.6 pin, which Warning As Error in NPOI.Benchmarks turns into an error and which currently takes down every PR in the repo. I've opened that separately as #1826 rather than bundling it here; this PR should go green once that lands.

(Comment from Claude, an AI assistant, on behalf of @ken-swyfft.)