fix(xlsb): consume unmatched workbook-global record bodies

[momomuchu]

Fixes #666.

read_workbook's record loop only consumes the body for BrtWbProp (0x0099)
and BrtBundleSh (0x009C) before checking for BrtEndBundleShs (0x0090).
Every other workbook-global record falls into _ => () and its body is
never read.

Because read_type() only consumes the type bytes, the next call reads
the unconsumed record's size varint as if it were a new type. For most
sizes this just self-heals a few bytes later, but a size of 144 or 156
encodes as a varint that reads back exactly as 0x0090 or 0x009C. So:

• size 144 gets misread as BrtEndBundleShs, the loop breaks before any
  real sheet is seen, and sheet_names() silently returns nothing.
• size 156 gets misread as BrtBundleSh, decoded from the wrong offset,
  and panics (index out of bounds).

This isn't limited to hand-crafted files. BrtAbsPath15 (the absolute
save-path record) is variable length and already present, unconsumed,
in this repo's own real Excel-produced fixtures (tests/issues.xlsb has
it at size 68). Its size depends only on how long the folder path was
when the file was saved, so a plain resave from a deep enough path can
land on 144 or 156 with no crafting involved.

The fix mirrors the existing 0x0099/0x009C arms: consume the body via
fill_buffer for any unmatched record too.

Added regression tests building minimal in-memory xlsb workbooks with a
synthetic unmatched record at the toxic sizes, plus a control case.

Note: there is a second, structurally identical unconsumed-body pattern
in the BrtName loop just below this one, in the same function. I left it
out to keep this fix focused on #666; happy to send a follow-up.

[jmcnamara]

Simplify this comment and don't mention the bug report number in the code since that requires the reader to go look up the issue. Instead explain briefly what non-obvious behaviour is happening like:

// Consume the body of any unmatched workbook-global record
// to avoid reading the trailing bytes in the next read.

[jmcnamara]

Don't build test files programmatically like this. It is better to have the xlsx/xlsb files in the tests directory so they can be examined or reused in other tests.

It also would be preferable to use files created by Excel. If the files are generated programmatically then there is no guarantee that what is being tested is valid. At a minimum the file should be opened and resaved in Excel.

[jmcnamara]

Don't use the issue name in the test title (here and elsewhere). Again this forces someone to go look up what issue #666 is. Give the test a descriptive title, then add a comment that explains what it is testing. At the end of the comment you can add See issue #666, but the reader shouldn't have to look it up for the current context.

[momomuchu]

Addressed all three:

• Simplified the comment at src/xlsb/mod.rs, no issue ref in the code.
• Gave the tests descriptive names (test_xlsb_unmatched_workbook_record_does_not_lose_sheet / _does_not_panic) with a // See issue #666 comment.
• Moved the test data out of the source into two fixture files under tests/, loaded with the usual wb(...) pattern, and removed the in-test byte builder.

On "produced by Excel": I don't have Excel, and LibreOffice here reads .xlsb but its export filter errors out, so I can't resave a real one. The two fixtures are minimal hand-built .xlsb files that reproduce both failure modes from #666 (both tests fail without the fix). Are those acceptable, or could you point me to / attach a real Excel-saved .xlsb with an unmatched workbook-global record so I can swap them in?

[jmcnamara]

On "produced by Excel": I don't have Excel, and LibreOffice here reads .xlsb but its export filter errors out, so I can't resave a real one.

My concern is that this PR is fixing an issue that doesn't happen in practice. It may happen in practice but that hasn't been demonstrated with a real file produced by Excel.

The original bug report at #666 says "affected real files are .xlsb exported from Microsoft Excel" but doesn't attach one of these file, even though that is a requirement for the bug report. I will ask again there.

[momomuchu]

Thanks for following up on #666, makes sense to get it from the source. I'll hold off touching the fixtures until we hear back from the reporter.

The bug itself (unmatched record body not consumed in read_workbook) is real independent of which file triggers it, the fixtures just reproduce the two failure modes from the report. If a real file surfaces a case they don't cover I'll adjust.

Anything else you want changed in the meantime, or are we just waiting on #666 now?

[jmcnamara]

Anything else you want changed in the meantime, or are we just waiting on #666 now?

Nothing to be done here. The fix looks good I just need to be sure that it is required. If there is no feedback on #666 I will merge it for the next release.