xls: fix issue with incorrect SST record

[jmcnamara]

This commit fixes an issue in the Excel xls SST record (Shared String Table) when the unique string count is incorrect.

Fixes #548

[Copilot]

Pull Request Overview

This PR fixes an issue in Excel xls file parsing where the SST (Shared String Table) record contains an incorrect unique string count, causing parsing to fail or miss strings.

• Modified the SST parsing logic to read all available strings instead of relying on the declared count
• Updated string parsing conditions to handle continuation records properly
• Added test coverage for the specific issue with incorrect SST unique count

Reviewed Changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.

File	Description
src/xls.rs	Updates SST parsing to read all strings until data is exhausted rather than trusting the count field
tests/test.rs	Adds test case to verify parsing of files with incorrect SST unique string counts
Changelog.md	Documents the bug fix for issue #548

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[jmcnamara]

@sftse If you get a chance could you review this Xls fix.

[timcoote]

documentation point: the breakage is in a .xls file, not xlsx.

should the user be made aware of the error in the xls file? I think that they should, but I don't know how calamine would do that (I'm almost totally ignorant of the package).

[sftse]

If the len is generally considered untrusted, can we remove the Vec::with_capacity(len) above it? This kind of rightsizing seems like a footgun, see #529, and would potentially lead to OOM if we had a fuzzer sophisticated enough to reach those codepaths.

Will try to give a more thorough review within a day or two.

[jmcnamara]

@timcoote

documentation point: the breakage is in a .xls file, not xlsx.

Thanks. Fixed.

should the user be made aware of the error in the xls file?

In this particular case I don't think the information is useful to most end-users and unless they wrote the xls exporting software there isn't anything that they can do about it. If you know what software produced these files we could raise the issue upstream.

[jmcnamara]

@sftse

If the len is generally considered untrusted, can we remove the Vec::with_capacity(len) above it? This kind of rightsizing seems like a footgun

Agreed. I removed that and rebased.

There are a lot of other similar instances in the code base. There are probably very few that have a measurable benefit to performance and some could led to crashes with deliberate or accidentally incorrect values in the parse file.

Will try to give a more thorough review within a day or two.

Thanks.

[timcoote]

In this particular case I don't think the information is useful to most end-users and unless they wrote the xls exporting software there isn't anything that they can do about it. If you know what software produced these files we could raise the issue upstream.

I've not yet bottomed out how this file was created. It comes from UK NHS. My concern is that such orgs have poor processes for keeping s/w current (or the export formats, for that matter: still on .xls), and anything that makes such mistakes more visible would reduce the risks from not updating often enough.
Reasonable to ignore.

What's my fastest route for validating the fix against other files - build the dev environment, or wait for the PR to flow thro' ?

[jmcnamara]

@timcoote

What's my fastest route for validating the fix against other files - build the dev environment, or wait for the PR to flow thro' ?

It depends on how urgently you need it. The fix should be released over the weekend at which point python-calamine can consume it.

If you want to test it in the interim you can modify the Cargo.toml file for python-calamine and modify the calamine dependency entry to the following:

calamine = { 
    version = "0.30.0", 
    features = ["dates"] , 
    git = "https://github.com/jmcnamara/calamine.git", 
    branch = "gh548_incorrect_sst_unique_count"}

You will then need to rebuild the python module and install it. This will be slightly tricky if you haven't done it before.

[timcoote]

Indeed. It failed on pytest - I'll chase up downstream. However, I also had to put the Cargo.toml dependency on one line for it to parse. Is this expected?

[jmcnamara]

Merged so that the fix can be released to unblock downstream.