File.ReadString doesn't predictably advance file when destination buffer is too short

[dysphie]

Help us help you

• [x] I have checked that my issue doesn't exist yet.
• [x] I have tried my absolute best to reduce the problem-space and have provided the absolute smallest test-case possible.
• [x] I can always reproduce the issue with the provided description below.

Environment

• Current SourceMod version: 1.11.0.6654

Description

Current behavior of File.ReadString makes it cumbersome to read indefinite length strings and data after it.

Problematic Code (or Steps to Reproduce)

Suppose we've just created a file. It contains an indeterminate length string, and a value after it:

File file = OpenFile("test_file", "wb");
file.WriteString("please_insert_batteries", true);
file.WriteInt32(123);
delete file;

Complementary code that reads from it:

File file = OpenFile("test_file", "rb");
char buffer[10]; // too small
file.ReadString(buffer, sizeof(buffer), -1);
int value;
file.ReadInt32(value);
PrintToServer("%d", value);
delete file;

Expected behavior:

file.ReadString reads the string, advancing the position in the file until a null terminator is found, copying the bytes into the buffer and discarding any excess. Then it reads the int32 value.

Actual behavior:

file.ReadString reads the string, advancing the position in the file until a null terminator is found or the buffer is consumed. If the latter happens the position in the file is now unclear. We can't safely read the int32 value.

[peace-maker]

A small buffer is a programming error or problem with the file format. You can use the return value of File.ReadString to check if you actually read the expected length and your buffer ends with a null. Silently throwing away data is not acceptable.

To make parsing such binary formats easier, you should write the length of the variable field before its actual data and use that to create your buffer.

[asherkin]

This was discussed on Discord before it was opened, I'm pretty confident the behaviour here is a bug - ReadString takes separate max_size and read_count params for a reason, it makes no sense for it stop reading before read_count is reached.

[dysphie]

You can use the return value of File.ReadString to check if you actually read the expected length and your buffer ends with a null

The problem is that the expected length can be any length and the buffer will always end with a null when read_count is -1, as per this code . Which means reading som or some_really_long_string into buffer[4] will both produce the same result, with no way of telling why the parser stopped reading (did we actually finish reading the string?)

To make parsing such binary formats easier, you should write the length of the variable field before its actual data and use that to create your buffer.

This would be ideal. Unfortunately the first code snippet was just an example, I don't actually control the contents of the files.

[asherkin]

ReadString takes separate max_size and read_count params for a reason, it makes no sense for it stop reading before read_count is reached.

This was all very wrong, I missed that read_count isn't being used in the example given (I think the case discussed previously was slightly different), and I now see that read_count isn't allowed to be bigger than max_size anyway. I do still think the behaviour here is wrong though, read_count = -1 mode probably needs to not stop reading from the file when the buffer is fully, but continue until it hits \0 or EOF.