Make cfread_string_len read no more than fits in the buffer

[dustinspace217]

What?

cfread_string_len now never reads more characters than fit in the buffer, and keeps the file position correct when it has to cut a string short.

Why?

The char* version's only length check was an Assertion, which is compiled out of release builds. A POF or pilot file with a string longer than the buffer was read past the end of it, which can crash release builds. A negative stored length wrote before the buffer.

How?

• Both versions clamp the stored length to what's left in the file before reading, so a length past the end keeps the bytes that are there, and a corrupt length can't cause a huge allocation or a seek past the end. Inside a limited pilot section, a corrupt length throws cfile::max_read_length before anything is read, so the pilot loaders still drop the section.
• The char* version keeps at most buffer_size - 1 characters, skips the rest so the next read lines up, and always terminates the buffer. A buffer size of 0 skips the string without writing to the buffer.
• A string cut to fit, a corrupt length or a failed read gets a Warning (debug builds; release builds keep FSO's convention of counting warnings only). A failed read seeks to the end of the string so later reads stay aligned.

Testing?

New CFileTest cases use in-memory files:

• a string that fits, and an exact fit with and without a stored terminator;
• a string longer than the buffer (checks nothing is written past it and the next value still reads correctly), and one with a terminator inside the buffer;
• zero and negative lengths, a length past the end of the file, and a corrupt length that is also cut;
• the read limit: a string ending exactly at the limit, one cut at it, and negative and too-long lengths under it, for both versions;
• a read that comes up short, using a file on disk shorter than its recorded size;
• a buffer size of 0;
• the SCP_string cases.

Each test also checks whether a warning was raised. Three of the original cases fail on the old code, and I broke each new rule on purpose and checked that a test failed. Release and Debug unit tests pass, and clang-tidy (CI's check) is clean.

I also checked it in game, on nightly 26.1.0-20261013 (b37cfda) with MediaVPs 5.0.2, SM1-01, in a mod holding only the two test models. A copy of a POF with one submodel's property string stretched to 8,000 bytes crashes a release build of the nightly in read_model_file_no_subsys (a debug build stops on the old Assertion). With this change the release build loads it with the same radius, bounding box, submodels and vertex counts as the unmodified file and the mission runs normally, and the debug build logs cfread_string_len: a string of 8000 characters was cut to fit a buffer of 256.

Anything Else?

Suggested by Goober5000 as the first step toward per-texture POF properties. I used Claude Code to write the fix and tests and to run the checks; I reviewed the code and this text.

[dustinspace217]

Made some fixes after an outside review raised some issues:

• An exact-fit string with no terminator is caught, and a cut string, a corrupt length or a failed read gets a Warning.
• The length is clamped to what's left in the file before reading, so a length past the end keeps the bytes that are there and the skip stays inside the file.
• Inside a limited pilot section, a corrupt length still throws cfile::max_read_length, so the loaders drop the section.
• A buffer size of 0 is no longer written to. Each case has a test.

[Goober5000]

Additional items

[Goober5000]

This function just adds noise. Write the Warning inline in the places that call it.

Also, at the calling sites, do not concatenate strings with std::to_string calls. Instead, use printf-style format arguments within the Warning itself.

[Goober5000]

putting parentheses around the sub-expressions would be good for readability

[Goober5000]

putting parentheses around the sub-expressions would be good for readability

[Goober5000]

putting parentheses around the sub-expressions would be good for readability

[dustinspace217]

Thanks for both passes. Two commits:

• bc6f9c1: the Warning helper is gone, each site calls Warning with format arguments, and the three conditions have parentheses around their sub-expressions.
• 030d667 takes the second-round items and the consolidation your review proposed: both overloads now share one core (read the length, work out what is stored, read the body, warn), so the failure handling exists once. With that: a negative length, a length past the limit, or a cut-off length field inside a limited section throws cfile::max_read_length even when the section ends at the end of the file (R2-1); a zero-size buffer still warns about a corrupt length (R2-2); a cut-off length field warns instead of reading as an empty string (R2-3); the header comments list the warning cases and the throw (R2-8). Four tests cover those.

Not taken: sharing cfread's size check (R2-4) would change cfread itself, which I'd rather keep out of this PR; the test helper duplication (R2-7) is two lines of little-endian encoding and I left it. Release and Debug suites are green.