TerryCavanagh / VVVVVV

The source code to VVVVVV! http://thelettervsixtim.es/
Other
7k stars 559 forks source link

Fix font .txt files not being null-terminated #947

Closed Daaaav closed 1 year ago

Daaaav commented 1 year ago

Changes:

This was an oversight when we migrated to the new UTF-8 system - it expects a null-terminated string, but the utfcpp implementation worked with a pointer to the end of the file instead.

I also added an assert in FILESYSTEM_loadFileToMemory() so this is less likely to happen again - because there should be no valid reason to have a NULL pointer for the total file size, as well as not wanting a null terminator to be added at the end of the file.

Legal Stuff:

By submitting this pull request, I confirm that...

InfoTeddy commented 1 year ago

Part of me wonders why we don't just always add a null byte. We already do that for the STDIN buffer after all. It would make things simpler and get rid of needing to deal with one argument when calling FILESYSTEM_loadFileToMemory/FILESYSTEM_loadAssetToMemory.

Daaaav commented 1 year ago

Yeah I agree - but apparently FIQ had a reason not to do that though. Maybe we need to read back some 2020 conversations about that.

InfoTeddy commented 1 year ago

Looks like in 5862af4445bfd9c056de7ef9694f8cb31f9d469a (#117), he wanted to specifically only do it for files loaded by TinyXML (which expect null termination). Presumably, he was just being cautious and didn't do it for any other files on the off chance that doing so would break something.

I can't think of any reason why always having a null terminator would break anything. On the Discord on January 24th, 2020, he said "always appending a null byte sounds bad", but... why?

Daaaav commented 1 year ago

I think his reasons were that it'd be cleaner not to add the null byte for binary files (like images), so that's why the boolean was added.

I personally still think it wouldn't be an issue to append a null byte at the end of binary files - we'd still return the file length without the extra byte to the caller, so the null should be considered outside the buffer boundaries when loading images and such anyway. Something might be said about the current way making it clear and explicit that a null byte is added by the file loading function (i.e. the caller doesn't think they have to realloc to add a null byte themselves or something). But this does result in code duplication and we can always just add a comment at the top of the function.