From 63bc71b796204ca4166bc092904e7c010b0071d9 Mon Sep 17 00:00:00 2001 From: Misa Date: Sat, 18 Mar 2023 15:12:24 -0700 Subject: [PATCH] Always add null terminator when loading files This removes the `addnull` argument from `FILESYSTEM_loadFileToMemory` and `FILESYSTEM_loadAssetToMemory`, and makes it so a null terminator is always appended no matter what. This simplifies things and removes the need for callers to make the decision about null termination and what its implications are. Then you get cases where null termination might not happen when it should be, such as the one df577c59ef16df8893e0628559de1b304e84b951 (#947) fixed. When FIQ added the `addnull` argument in 5862af4445bfd9c056de7ef9694f8cb31f9d469a (#117), I'm guessing he did it because he wanted to be cautious about adding the null terminator to every file, so he only did it for XML files, which was the only case needed at the time. But really, there's no downsides to always appending a null terminator. In fact, it's already always done whenever the STDIN buffer is loaded. --- desktop_version/src/CustomLevels.cpp | 2 +- desktop_version/src/FileSystemUtils.cpp | 42 +++++++---------------- desktop_version/src/FileSystemUtils.h | 5 ++- desktop_version/src/Font.cpp | 2 +- desktop_version/src/GraphicsResources.cpp | 2 +- desktop_version/src/Music.cpp | 2 +- 6 files changed, 18 insertions(+), 37 deletions(-) diff --git a/desktop_version/src/CustomLevels.cpp b/desktop_version/src/CustomLevels.cpp index 2c93581e..4fc48772 100644 --- a/desktop_version/src/CustomLevels.cpp +++ b/desktop_version/src/CustomLevels.cpp @@ -286,7 +286,7 @@ void customlevelclass::getDirectoryData(void) bool customlevelclass::getLevelMetaDataAndPlaytestArgs(const std::string& _path, LevelMetaData& _data, CliPlaytestArgs* pt_args) { unsigned char *uMem; - FILESYSTEM_loadFileToMemory(_path.c_str(), &uMem, NULL, true); + FILESYSTEM_loadFileToMemory(_path.c_str(), &uMem, NULL); if (uMem == NULL) { diff --git a/desktop_version/src/FileSystemUtils.cpp b/desktop_version/src/FileSystemUtils.cpp index 3a706de1..2052939c 100644 --- a/desktop_version/src/FileSystemUtils.cpp +++ b/desktop_version/src/FileSystemUtils.cpp @@ -697,8 +697,7 @@ static void load_stdin(void) bool end = ch == EOF; if (end) { - /* Add null terminator. There's no observable change in - * behavior if addnull is always true, but not vice versa. */ + /* Always add null terminator. */ ch = '\0'; } @@ -729,8 +728,7 @@ static void load_stdin(void) void FILESYSTEM_loadFileToMemory( const char *name, unsigned char **mem, - size_t *len, - bool addnull + size_t *len ) { PHYSFS_File *handle; PHYSFS_sint64 length; @@ -741,12 +739,6 @@ void FILESYSTEM_loadFileToMemory( goto fail; } - if (len == NULL && !addnull) - { - vlog_warn("%s is loaded with len == NULL && !addnull", name); - SDL_assert(0 && "Are you sure you don't want a null terminator to be added to these loaded file contents?"); - } - /* FIXME: Dumb hack to use `special/stdin.vvvvvv` here... * This is also checked elsewhere... grep for `special/stdin`! */ if (SDL_strcmp(name, "levels/special/stdin.vvvvvv") == 0) @@ -791,23 +783,14 @@ void FILESYSTEM_loadFileToMemory( } *len = length; } - if (addnull) + + *mem = (unsigned char *) SDL_malloc(length + 1); + if (*mem == NULL) { - *mem = (unsigned char *) SDL_malloc(length + 1); - if (*mem == NULL) - { - VVV_exit(1); - } - (*mem)[length] = 0; - } - else - { - *mem = (unsigned char*) SDL_malloc(length); - if (*mem == NULL) - { - VVV_exit(1); - } + VVV_exit(1); } + (*mem)[length] = 0; + success = PHYSFS_readBytes(handle, *mem, length); if (success == -1) { @@ -830,14 +813,13 @@ fail: void FILESYSTEM_loadAssetToMemory( const char* name, unsigned char** mem, - size_t* len, - const bool addnull + size_t* len ) { char path[MAX_PATH]; getMountedPath(path, sizeof(path), name); - FILESYSTEM_loadFileToMemory(path, mem, len, addnull); + FILESYSTEM_loadFileToMemory(path, mem, len); } bool FILESYSTEM_loadBinaryBlob(binaryBlob* blob, const char* filename) @@ -980,7 +962,7 @@ bool FILESYSTEM_loadTiXml2Document(const char *name, tinyxml2::XMLDocument& doc) { /* XMLDocument.LoadFile doesn't account for Unicode paths, PHYSFS does */ unsigned char *mem; - FILESYSTEM_loadFileToMemory(name, &mem, NULL, true); + FILESYSTEM_loadFileToMemory(name, &mem, NULL); if (mem == NULL) { return false; @@ -994,7 +976,7 @@ bool FILESYSTEM_loadAssetTiXml2Document(const char *name, tinyxml2::XMLDocument& { /* Same as FILESYSTEM_loadTiXml2Document except for possible custom assets */ unsigned char *mem; - FILESYSTEM_loadAssetToMemory(name, &mem, NULL, true); + FILESYSTEM_loadAssetToMemory(name, &mem, NULL); if (mem == NULL) { return false; diff --git a/desktop_version/src/FileSystemUtils.h b/desktop_version/src/FileSystemUtils.h index f986c6db..bd564d7e 100644 --- a/desktop_version/src/FileSystemUtils.h +++ b/desktop_version/src/FileSystemUtils.h @@ -31,12 +31,11 @@ bool FILESYSTEM_isAssetMounted(const char* filename); bool FILESYSTEM_areAssetsInSameRealDir(const char* filenameA, const char* filenameB); void FILESYSTEM_loadFileToMemory(const char *name, unsigned char **mem, - size_t *len, bool addnull); + size_t *len); void FILESYSTEM_loadAssetToMemory( const char* name, unsigned char** mem, - size_t* len, - const bool addnull + size_t* len ); bool FILESYSTEM_loadBinaryBlob(binaryBlob* blob, const char* filename); diff --git a/desktop_version/src/Font.cpp b/desktop_version/src/Font.cpp index 1e03477a..d08e9cb8 100644 --- a/desktop_version/src/Font.cpp +++ b/desktop_version/src/Font.cpp @@ -349,7 +349,7 @@ static uint8_t load_font(FontContainer* container, const char* name) { /* The .txt can contain null bytes, but it's still null-terminated - it protects * against incomplete sequences getting the UTF-8 decoder to read out of bounds. */ - FILESYSTEM_loadAssetToMemory(name_txt, &charmap, &length, true); + FILESYSTEM_loadAssetToMemory(name_txt, &charmap, &length); } if (charmap != NULL) { diff --git a/desktop_version/src/GraphicsResources.cpp b/desktop_version/src/GraphicsResources.cpp index 0da78040..99b03cd8 100644 --- a/desktop_version/src/GraphicsResources.cpp +++ b/desktop_version/src/GraphicsResources.cpp @@ -29,7 +29,7 @@ static SDL_Surface* LoadImageRaw(const char* filename, unsigned char** data) unsigned char* fileIn; size_t length; - FILESYSTEM_loadAssetToMemory(filename, &fileIn, &length, false); + FILESYSTEM_loadAssetToMemory(filename, &fileIn, &length); if (fileIn == NULL) { SDL_assert(0 && "Image file missing!"); diff --git a/desktop_version/src/Music.cpp b/desktop_version/src/Music.cpp index 9c9ee0aa..61c0ca6f 100644 --- a/desktop_version/src/Music.cpp +++ b/desktop_version/src/Music.cpp @@ -100,7 +100,7 @@ public: SDL_AudioSpec spec; SDL_RWops *fileIn; SDL_zerop(this); - FILESYSTEM_loadAssetToMemory(fileName, &mem, &length, false); + FILESYSTEM_loadAssetToMemory(fileName, &mem, &length); if (mem == NULL) { vlog_error("Unable to load WAV file %s", fileName);