Skip to content

xlsb: don't name a sheet of this workbook for a reference into another - #717

Open
sjvrensburg wants to merge 1 commit into
tafia:masterfrom
sjvrensburg:xlsb-external-supbooks
Open

xlsb: don't name a sheet of this workbook for a reference into another#717
sjvrensburg wants to merge 1 commit into
tafia:masterfrom
sjvrensburg:xlsb-external-supbooks

Conversation

@sjvrensburg

Copy link
Copy Markdown

Disclosure: this pull request was written by an AI agent (Claude, via Claude
Code), including the code, the tests and this description. I directed and
reviewed the work, and I have run the tests and checks reported below, but I
did not hand-write the patch. I am raising it because the underlying bug is
real and reproducible, and the evidence is checkable independently of who or
what wrote it. If you do not accept AI-generated contributions, please close
this
— no hard feelings, and I would be glad to file it as an issue with the
findings instead so someone else can pick it up.

Summary

An Xti carries two indices: a supporting book, and a tab within that book.
BrtExternSheet used only the second, resolving it against the sheets of the
workbook being read:

match read_i32(&xti[4..8]) {
    -2 => "#ThisWorkbook",
    -1 => "#InvalidWorkSheet",
    p if p >= 0 && (p as usize) < sheets.len() => &sheets[p as usize].0,
    _ => "#Unknown",
}

Why it matters

For a workbook that links to another one — last year's copy of itself, most
commonly — this names a real sheet of ours, records a real dependency, and
points it at the wrong place. Nothing about the result looks wrong. The
reference reads SHEET_A!$D$21, SHEET_A exists in this workbook, and $D$21
is a cell on it. A caller has no way to tell that the formula was never talking
about this workbook at all.

On the workbook I found it on, the externals block declares four supporting
books — BrtSupSelf (0x0165) first, then three BrtSupBookSrc (0x0163) —
and its 18 Xti entries name them:

ixti supbook tab resolved to actually
2 1 5 SHEET_A sheet 6 of another workbook
6 2 12 SHEET_B sheet 13 of another workbook
16 3 26 #Unknown sheet 27 of another workbook

Twelve formulas used them. Ten disagreed with the value Excel had stored; two
agreed by coincidence, the local cell happening to hold the same number as the
foreign one — which is the case that would never have been found by looking.

The evidence is arithmetic, not documentary

One of those formulas reads

=IF($B2="ACTIVE",SHEET_A!$D$21,SHEET_A!$D$22)

and Excel stored 2. SHEET_A is 13 rows long, so $D$21 is empty and the
formula could not have produced 2 from it. SHEET_C!D21:D24 — the sheet the
self-book Xti at index 3 names — holds 2, 0, 2, 1, which is exactly what
all six formulas of that family stored. The supporting books are copies of this
workbook, and the sheet in question sits at a different tab index in them.

(Sheet names are pseudonymised; the workbook is confidential. The arithmetic is
unaffected.)

The fix

Supporting books are tracked in declaration order, and a tab index resolves
against our sheets only when the Xti actually names ours. A reference into
another book is written [1]#Sheet3 — the shape an external reference already
has in this codebase — because the sheet's own name lives in that workbook and
it is not open. A workbook that declares no supporting books keeps the old
behaviour, every reference being local. The two sentinels still outrank the
book index.

xti_sheet is a free function with unit tests for each case.

On test coverage, honestly

No fixture has an external link and I could not author one, for the usual
reason: no open-source tool writes XLSB. The end-to-end evidence is the
workbook above, which I cannot share — hence the arithmetic, which stands on
its own.

After this change, recomputing every formula in that workbook left zero
disagreements out of 6,677,397 evaluable formulas, down from 11. What remains
unevaluated there is two unimplemented functions and the 12 references into
workbooks that are not open — honest gaps, no decoding failures.

If you can supply or generate a .xlsb with an external link, I will add it.

Independent of #712, #713 and my other branches; branched from master.

All 274 tests pass; cargo fmt --check and cargo clippy --all-targets are
clean.

An `Xti` carries two indices: a supporting book, and a tab within that
book. `BrtExternSheet` used only the second, resolving it against this
workbook's sheets whatever the first said.

For a workbook that links to another one — last year's copy of itself,
most commonly — that names a real sheet of ours, records a real
dependency, and points it at the wrong place. Nothing about the result
looks wrong. The reference reads `Journal!$D$21`, `Journal` exists, and
`$D$21` is a cell.

On a workbook whose values can be checked, 18 `Xti` entries across four
supporting books: `BrtSupSelf` first, then three `BrtSupBookSrc`. The
three external ones were resolving to `Journal`, to a pivot sheet, and —
its tab index being past our last sheet — to `#Unknown`. Ten formulas
computed from them, and every one disagreed with the value Excel had
stored. The formula our reader gave as

    =IF($D2="O",Journal!$D$23,Journal!$D$24)

reads `$D$23` and `$D$24` of a 13-row sheet. Excel stored 1, which is
what those cells hold in the *other* workbook.

So supporting books are now tracked in declaration order — `BrtSupSelf`
(0x0165) for this workbook, `BrtSupBookSrc` (0x0163) for another — and a
tab index resolves against our sheets only when the `Xti` names ours. A
reference into another book is written `[1]#Sheet3`, in the shape an
external reference already has, because the sheet's own name lives in
that workbook and it is not open. A workbook that declares no supporting
books at all keeps the old behaviour, every reference being local.

The two existing sentinels still outrank the book index, and a tab past
our last sheet is still `#Unknown`.

No fixture has an external link, and none can be authored for `.xlsb`.
The resolution is a free function with unit tests for each case; the
end-to-end evidence is that workbook, where these ten formulas stop
being read as local dependencies.
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