From a17f7b5c2f22c84b4b73178f3fee5fe492ef8c88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Tue, 4 Jun 2024 00:00:04 +0200 Subject: [PATCH] Rewrite and fix line reader, read entire file into memory Read the entire file into memory immediately when the line reader is initialized instead of using a fixed size buffer of size 32769, which leads to broken line reading for larger files (closes #8431). Replace the `CLineReader::Init` function with the `CLineReader::OpenFile` function, which additionally checks whether the file contains any null bytes (indicates that the file is likely not a text file). As the file will be read into memory entirely in the `OpenFile` function, is can also be closed immediately, since using the `io_read_all_str` function should ensure that nothing more can be read from the file. This also simplifies the usage of the `CLineReader` class, as manually closing the file is inconvenient and error-prone. In fact, the file handle for the `ddnet-serverlist-urls.cfg` file was previously not closed properly. Benchmarking on Windows did not show any noticeable performance impact of this change both for smaller files (config and language files) and larger files (synthetic ~10 MiB files). Let the `CLineReader::Get` function return `const char *` instead of `char *`, since users of this class should not modify the internal buffer. Also, since the entire file is read into memory now, the returned strings are now valid until the respective line reader object is destructed, instead of only until the `Get` function is called again. This simplifies the usage in some cases where all lines are handled, since additional temporary buffers are now unnecessary. Remove the `IOFLAG_SKIP_BOM` flag from the `io_open` function, as this flag was only used together with the line reader. This was inconvenient to use, as any use of the `io_open` function specifically for line readers had to be used with `IOFLAG_SKIP_BOM`. Now, the line reader transparently skips the UTF-8 BOM internally. Skipping the UTF-8 BOM never worked with the `IStorage::ReadFileStr` function, because `io_length` is used in the `io_read_all` function, which rewinds the file position to before the UTF-8 BOM that was skipped by using `IOFLAG_SKIP_BOM`. In any case, as the `ReadFileStr` function is currently unused, this has/had no further effect. The respective test cases for `IOFLAG_SKIP_BOM` are removed. Add more test cases for the `CLineReader` class. All test cases are checked both with and without the UTF-8 BOM. Additional tests with mixed new lines, empty lines with different new lines, and internal null bytes are added. Consistently use the construct `while(const char *pLine = LineReader.Get())` to iterate over all lines of a line reader. In this case, the assignment inside the `while`-loop seems acceptable over the alternatives, e.g. `while(true)` with `break` or adding a function `CLineReader::ForAll(std::function Consumer)`. --- src/android/android_main.cpp | 10 +- src/base/system.cpp | 19 +-- src/base/system.h | 3 +- .../client/backend/opengl/opengl_sl.cpp | 10 +- src/engine/client/serverbrowser_http.cpp | 11 +- src/engine/server/server.cpp | 18 +- src/engine/shared/console.cpp | 14 +- src/engine/shared/linereader.cpp | 161 +++++++++--------- src/engine/shared/linereader.h | 17 +- src/engine/shared/storage.cpp | 18 +- src/game/client/components/countryflags.cpp | 13 +- src/game/editor/auto_map.cpp | 11 +- src/game/localization.cpp | 24 +-- src/game/server/gamecontext.cpp | 44 ++--- src/game/server/score.cpp | 10 +- src/test/io.cpp | 43 ++--- src/test/linereader.cpp | 82 ++++++--- src/tools/config_store.cpp | 27 +-- 18 files changed, 240 insertions(+), 295 deletions(-) diff --git a/src/android/android_main.cpp b/src/android/android_main.cpp index 633c30443..558c130d0 100644 --- a/src/android/android_main.cpp +++ b/src/android/android_main.cpp @@ -120,17 +120,14 @@ public: static std::vector ReadIntegrityFile(const char *pFilename) { - IOHANDLE IntegrityFile = io_open(pFilename, IOFLAG_READ); - if(!IntegrityFile) + CLineReader LineReader; + if(!LineReader.OpenFile(io_open(pFilename, IOFLAG_READ))) { return {}; } - CLineReader LineReader; - LineReader.Init(IntegrityFile); - const char *pReadLine; std::vector vLines; - while((pReadLine = LineReader.Get())) + while(const char *pReadLine = LineReader.Get()) { const char *pSpaceInLine = str_rchr(pReadLine, ' '); CIntegrityFileLine Line; @@ -159,7 +156,6 @@ static std::vector ReadIntegrityFile(const char *pFilename) vLines.emplace_back(std::move(Line)); } - io_close(IntegrityFile); return vLines; } diff --git a/src/base/system.cpp b/src/base/system.cpp index a7e6a2b88..56df4589c 100644 --- a/src/base/system.cpp +++ b/src/base/system.cpp @@ -251,9 +251,9 @@ bool mem_has_null(const void *block, size_t size) return false; } -IOHANDLE io_open_impl(const char *filename, int flags) +IOHANDLE io_open(const char *filename, int flags) { - dbg_assert(flags == (IOFLAG_READ | IOFLAG_SKIP_BOM) || flags == IOFLAG_READ || flags == IOFLAG_WRITE || flags == IOFLAG_APPEND, "flags must be read, read+skipbom, write or append"); + dbg_assert(flags == IOFLAG_READ || flags == IOFLAG_WRITE || flags == IOFLAG_APPEND, "flags must be read, write or append"); #if defined(CONF_FAMILY_WINDOWS) const std::wstring wide_filename = windows_utf8_to_wide(filename); DWORD desired_access; @@ -313,21 +313,6 @@ IOHANDLE io_open_impl(const char *filename, int flags) #endif } -IOHANDLE io_open(const char *filename, int flags) -{ - IOHANDLE result = io_open_impl(filename, flags); - unsigned char buf[3]; - if((flags & IOFLAG_SKIP_BOM) == 0 || !result) - { - return result; - } - if(io_read(result, buf, sizeof(buf)) != 3 || buf[0] != 0xef || buf[1] != 0xbb || buf[2] != 0xbf) - { - io_seek(result, 0, IOSEEK_START); - } - return result; -} - unsigned io_read(IOHANDLE io, void *buffer, unsigned size) { return fread(buffer, 1, size, (FILE *)io); diff --git a/src/base/system.h b/src/base/system.h index bcb3b46e2..704e89356 100644 --- a/src/base/system.h +++ b/src/base/system.h @@ -222,7 +222,6 @@ enum IOFLAG_READ = 1, IOFLAG_WRITE = 2, IOFLAG_APPEND = 4, - IOFLAG_SKIP_BOM = 8, IOSEEK_START = 0, IOSEEK_CUR = 1, @@ -237,7 +236,7 @@ enum * @param File to open. * @param flags A set of IOFLAG flags. * - * @sa IOFLAG_READ, IOFLAG_WRITE, IOFLAG_APPEND, IOFLAG_SKIP_BOM. + * @sa IOFLAG_READ, IOFLAG_WRITE, IOFLAG_APPEND. * * @return A handle to the file on success and 0 on failure. * diff --git a/src/engine/client/backend/opengl/opengl_sl.cpp b/src/engine/client/backend/opengl/opengl_sl.cpp index c955040d1..a8b16c6af 100644 --- a/src/engine/client/backend/opengl/opengl_sl.cpp +++ b/src/engine/client/backend/opengl/opengl_sl.cpp @@ -23,10 +23,10 @@ bool CGLSL::LoadShader(CGLSLCompiler *pCompiler, IStorage *pStorage, const char { if(m_IsLoaded) return true; - IOHANDLE f = pStorage->OpenFile(pFile, IOFLAG_READ | IOFLAG_SKIP_BOM, IStorage::TYPE_ALL); + CLineReader LineReader; std::vector vLines; - if(f) + if(LineReader.OpenFile(pStorage->OpenFile(pFile, IOFLAG_READ, IStorage::TYPE_ALL))) { EBackendType BackendType = pCompiler->m_IsOpenGLES ? BACKEND_TYPE_OPENGL_ES : BACKEND_TYPE_OPENGL; bool IsNewOpenGL = (BackendType == BACKEND_TYPE_OPENGL ? (pCompiler->m_OpenGLVersionMajor >= 4 || (pCompiler->m_OpenGLVersionMajor == 3 && pCompiler->m_OpenGLVersionMinor == 3)) : pCompiler->m_OpenGLVersionMajor >= 3); @@ -81,17 +81,13 @@ bool CGLSL::LoadShader(CGLSLCompiler *pCompiler, IStorage *pStorage, const char vLines.emplace_back("#extension GL_EXT_texture_array : enable\r\n"); } - CLineReader LineReader; - LineReader.Init(f); - char *pReadLine = NULL; - while((pReadLine = LineReader.Get())) + while(const char *pReadLine = LineReader.Get()) { std::string Line; pCompiler->ParseLine(Line, pReadLine, Type == GL_FRAGMENT_SHADER ? GLSL_SHADER_COMPILER_TYPE_FRAGMENT : GLSL_SHADER_COMPILER_TYPE_VERTEX); Line.append("\r\n"); vLines.push_back(Line); } - io_close(f); const char **ShaderCode = new const char *[vLines.size()]; diff --git a/src/engine/client/serverbrowser_http.cpp b/src/engine/client/serverbrowser_http.cpp index dae761de8..293720587 100644 --- a/src/engine/client/serverbrowser_http.cpp +++ b/src/engine/client/serverbrowser_http.cpp @@ -540,15 +540,12 @@ IServerBrowserHttp *CreateServerBrowserHttp(IEngine *pEngine, IStorage *pStorage const char *apUrls[CChooseMaster::MAX_URLS] = {0}; const char **ppUrls = apUrls; int NumUrls = 0; - IOHANDLE File = pStorage->OpenFile("ddnet-serverlist-urls.cfg", IOFLAG_READ | IOFLAG_SKIP_BOM, IStorage::TYPE_ALL); - if(File) + CLineReader LineReader; + if(LineReader.OpenFile(pStorage->OpenFile("ddnet-serverlist-urls.cfg", IOFLAG_READ, IStorage::TYPE_ALL))) { - CLineReader Lines; - Lines.Init(File); - while(NumUrls < CChooseMaster::MAX_URLS) + while(const char *pLine = LineReader.Get()) { - const char *pLine = Lines.Get(); - if(!pLine) + if(NumUrls == CChooseMaster::MAX_URLS) { break; } diff --git a/src/engine/server/server.cpp b/src/engine/server/server.cpp index 85278f82e..b33d6dc0e 100644 --- a/src/engine/server/server.cpp +++ b/src/engine/server/server.cpp @@ -3860,18 +3860,18 @@ const char *CServer::GetAnnouncementLine(const char *pFileName) str_copy(m_aAnnouncementFile, pFileName); m_vAnnouncements.clear(); - IOHANDLE File = m_pStorage->OpenFile(pFileName, IOFLAG_READ | IOFLAG_SKIP_BOM, IStorage::TYPE_ALL); - if(!File) + CLineReader LineReader; + if(!LineReader.OpenFile(m_pStorage->OpenFile(pFileName, IOFLAG_READ, IStorage::TYPE_ALL))) + { return 0; - - char *pLine; - CLineReader Reader; - Reader.Init(File); - while((pLine = Reader.Get())) + } + while(const char *pLine = LineReader.Get()) + { if(str_length(pLine) && pLine[0] != '#') + { m_vAnnouncements.emplace_back(pLine); - - io_close(File); + } + } } if(m_vAnnouncements.empty()) diff --git a/src/engine/shared/console.cpp b/src/engine/shared/console.cpp index 7d2e565a7..29610d796 100644 --- a/src/engine/shared/console.cpp +++ b/src/engine/shared/console.cpp @@ -622,23 +622,19 @@ bool CConsole::ExecuteFile(const char *pFilename, int ClientId, bool LogFailure, m_pFirstExec = &ThisFile; // exec the file - IOHANDLE File = m_pStorage->OpenFile(pFilename, IOFLAG_READ | IOFLAG_SKIP_BOM, StorageType); - + CLineReader LineReader; bool Success = false; char aBuf[32 + IO_MAX_PATH_LENGTH]; - if(File) + if(LineReader.OpenFile(m_pStorage->OpenFile(pFilename, IOFLAG_READ, StorageType))) { str_format(aBuf, sizeof(aBuf), "executing '%s'", pFilename); Print(IConsole::OUTPUT_LEVEL_STANDARD, "console", aBuf); - CLineReader Reader; - Reader.Init(File); - - char *pLine; - while((pLine = Reader.Get())) + while(const char *pLine = LineReader.Get()) + { ExecuteLine(pLine, ClientId); + } - io_close(File); Success = true; } else if(LogFailure) diff --git a/src/engine/shared/linereader.cpp b/src/engine/shared/linereader.cpp index 9bc62d8ba..ae56cc39f 100644 --- a/src/engine/shared/linereader.cpp +++ b/src/engine/shared/linereader.cpp @@ -4,97 +4,92 @@ #include -void CLineReader::Init(IOHANDLE File) +CLineReader::CLineReader() { - m_BufferMaxSize = sizeof(m_aBuffer) - 1; - m_BufferSize = 0; - m_BufferPos = 0; - m_File = File; + m_pBuffer = nullptr; } -char *CLineReader::Get() +CLineReader::~CLineReader() { - unsigned LineStart = m_BufferPos; - bool CRLFBreak = false; + free(m_pBuffer); +} - while(true) +bool CLineReader::OpenFile(IOHANDLE File) +{ + if(!File) { - if(m_BufferPos >= m_BufferSize) - { - // fetch more + return false; + } + char *pBuffer = io_read_all_str(File); + io_close(File); + if(pBuffer == nullptr) + { + return false; + } + OpenBuffer(pBuffer); + return true; +} - // move the remaining part to the front - unsigned Left = m_BufferSize - LineStart; +void CLineReader::OpenBuffer(char *pBuffer) +{ + dbg_assert(pBuffer != nullptr, "Line reader initialized without valid buffer"); - if(LineStart > m_BufferSize) - Left = 0; - if(Left) - mem_move(m_aBuffer, &m_aBuffer[LineStart], Left); - m_BufferPos = Left; + m_pBuffer = pBuffer; + m_BufferPos = 0; + m_ReadLastLine = false; - // fill the buffer - unsigned Read = io_read(m_File, &m_aBuffer[m_BufferPos], m_BufferMaxSize - m_BufferPos); - m_BufferSize = Left + Read; - LineStart = 0; - - if(!Read) - { - if(Left) - { - m_aBuffer[Left] = '\0'; // return the last line - m_BufferPos = Left; - m_BufferSize = Left; - if(!str_utf8_check(m_aBuffer)) - { - LineStart = m_BufferPos; - CRLFBreak = false; - continue; // skip lines containing invalid UTF-8 - } - return m_aBuffer; - } - return nullptr; // we are done! - } - } - else - { - if(m_aBuffer[m_BufferPos] == '\n' || m_aBuffer[m_BufferPos] == '\r') - { - // line found - if(m_aBuffer[m_BufferPos] == '\r') - { - if(m_BufferPos + 1 >= m_BufferSize) - { - // read more to get the connected '\n' - CRLFBreak = true; - ++m_BufferPos; - continue; - } - else if(m_aBuffer[m_BufferPos + 1] == '\n') - m_aBuffer[m_BufferPos++] = '\0'; - } - m_aBuffer[m_BufferPos++] = '\0'; - if(!str_utf8_check(&m_aBuffer[LineStart])) - { - LineStart = m_BufferPos; - CRLFBreak = false; - continue; // skip lines containing invalid UTF-8 - } - return &m_aBuffer[LineStart]; - } - else if(CRLFBreak) - { - if(m_aBuffer[m_BufferPos] == '\n') - m_aBuffer[m_BufferPos++] = '\0'; - if(!str_utf8_check(&m_aBuffer[LineStart])) - { - LineStart = m_BufferPos; - CRLFBreak = false; - continue; // skip lines containing invalid UTF-8 - } - return &m_aBuffer[LineStart]; - } - else - m_BufferPos++; - } + // Skip UTF-8 BOM + if(m_pBuffer[0] == '\xEF' && m_pBuffer[1] == '\xBB' && m_pBuffer[2] == '\xBF') + { + m_BufferPos += 3; + } +} + +const char *CLineReader::Get() +{ + dbg_assert(m_pBuffer != nullptr, "Line reader not initialized"); + if(m_ReadLastLine) + { + return nullptr; + } + + unsigned LineStart = m_BufferPos; + while(true) + { + if(m_pBuffer[m_BufferPos] == '\0' || m_pBuffer[m_BufferPos] == '\n' || (m_pBuffer[m_BufferPos] == '\r' && m_pBuffer[m_BufferPos + 1] == '\n')) + { + if(m_pBuffer[m_BufferPos] == '\0') + { + m_ReadLastLine = true; + } + else + { + if(m_pBuffer[m_BufferPos] == '\r') + { + m_pBuffer[m_BufferPos] = '\0'; + ++m_BufferPos; + } + m_pBuffer[m_BufferPos] = '\0'; + ++m_BufferPos; + } + + if(!str_utf8_check(&m_pBuffer[LineStart])) + { + // Skip lines containing invalid UTF-8 + if(m_ReadLastLine) + { + return nullptr; + } + LineStart = m_BufferPos; + continue; + } + // Skip trailing empty line + if(m_ReadLastLine && m_pBuffer[LineStart] == '\0') + { + return nullptr; + } + return &m_pBuffer[LineStart]; + } + ++m_BufferPos; } } diff --git a/src/engine/shared/linereader.h b/src/engine/shared/linereader.h index 7d5f1c5ab..4575b7659 100644 --- a/src/engine/shared/linereader.h +++ b/src/engine/shared/linereader.h @@ -4,17 +4,20 @@ #define ENGINE_SHARED_LINEREADER_H #include -// buffered stream for reading lines, should perhaps be something smaller +// buffered stream for reading lines class CLineReader { - char m_aBuffer[4 * 8192 + 1]; // 1 additional byte for null termination + char *m_pBuffer; unsigned m_BufferPos; - unsigned m_BufferSize; - unsigned m_BufferMaxSize; - IOHANDLE m_File; + bool m_ReadLastLine; public: - void Init(IOHANDLE File); - char *Get(); // Returned string is only valid until next Get() call + CLineReader(); + ~CLineReader(); + + bool OpenFile(IOHANDLE File); + void OpenBuffer(char *pBuffer); // Buffer must have been allocated with malloc, will be freed by the line reader + + const char *Get(); // Returned string is valid until the line reader is destroyed }; #endif diff --git a/src/engine/shared/storage.cpp b/src/engine/shared/storage.cpp index 44401fbf2..14bf3e197 100644 --- a/src/engine/shared/storage.cpp +++ b/src/engine/shared/storage.cpp @@ -127,7 +127,7 @@ public: void LoadPaths(const char *pArgv0) { // check current directory - IOHANDLE File = io_open("storage.cfg", IOFLAG_READ | IOFLAG_SKIP_BOM); + IOHANDLE File = io_open("storage.cfg", IOFLAG_READ); if(!File) { // check usable path in argv[0] @@ -140,7 +140,7 @@ public: char aBuffer[IO_MAX_PATH_LENGTH]; str_copy(aBuffer, pArgv0, Pos + 1); str_append(aBuffer, "/storage.cfg"); - File = io_open(aBuffer, IOFLAG_READ | IOFLAG_SKIP_BOM); + File = io_open(aBuffer, IOFLAG_READ); } if(Pos >= IO_MAX_PATH_LENGTH || !File) @@ -151,10 +151,12 @@ public: } CLineReader LineReader; - LineReader.Init(File); - - char *pLine; - while((pLine = LineReader.Get())) + if(!LineReader.OpenFile(File)) + { + dbg_msg("storage", "couldn't open storage.cfg"); + return; + } + while(const char *pLine = LineReader.Get()) { const char *pLineWithoutPrefix = str_startswith(pLine, "add_path "); if(pLineWithoutPrefix) @@ -163,8 +165,6 @@ public: } } - io_close(File); - if(!m_NumPaths) dbg_msg("storage", "no paths found in storage.cfg"); } @@ -565,7 +565,7 @@ public: char *ReadFileStr(const char *pFilename, int Type) override { - IOHANDLE File = OpenFile(pFilename, IOFLAG_READ | IOFLAG_SKIP_BOM, Type); + IOHANDLE File = OpenFile(pFilename, IOFLAG_READ, Type); if(!File) return nullptr; char *pResult = io_read_all_str(File); diff --git a/src/game/client/components/countryflags.cpp b/src/game/client/components/countryflags.cpp index 8de2024ea..b05778596 100644 --- a/src/game/client/components/countryflags.cpp +++ b/src/game/client/components/countryflags.cpp @@ -16,8 +16,8 @@ void CCountryFlags::LoadCountryflagsIndexfile() { const char *pFilename = "countryflags/index.txt"; - IOHANDLE File = Storage()->OpenFile(pFilename, IOFLAG_READ | IOFLAG_SKIP_BOM, IStorage::TYPE_ALL); - if(!File) + CLineReader LineReader; + if(!LineReader.OpenFile(Storage()->OpenFile(pFilename, IOFLAG_READ, IStorage::TYPE_ALL))) { char aBuf[128]; str_format(aBuf, sizeof(aBuf), "couldn't open index file '%s'", pFilename); @@ -26,16 +26,13 @@ void CCountryFlags::LoadCountryflagsIndexfile() } char aOrigin[128]; - CLineReader LineReader; - LineReader.Init(File); - char *pLine; - while((pLine = LineReader.Get())) + while(const char *pLine = LineReader.Get()) { if(!str_length(pLine) || pLine[0] == '#') // skip empty lines and comments continue; str_copy(aOrigin, pLine); - char *pReplacement = LineReader.Get(); + const char *pReplacement = LineReader.Get(); if(!pReplacement) { Console()->Print(IConsole::OUTPUT_LEVEL_ADDINFO, "countryflags", "unexpected end of index file"); @@ -84,7 +81,7 @@ void CCountryFlags::LoadCountryflagsIndexfile() } m_vCountryFlags.push_back(CountryFlag); } - io_close(File); + std::sort(m_vCountryFlags.begin(), m_vCountryFlags.end()); // find index of default item diff --git a/src/game/editor/auto_map.cpp b/src/game/editor/auto_map.cpp index ca38934c6..8567f7cd6 100644 --- a/src/game/editor/auto_map.cpp +++ b/src/game/editor/auto_map.cpp @@ -49,8 +49,8 @@ void CAutoMapper::Load(const char *pTileName) { char aPath[IO_MAX_PATH_LENGTH]; str_format(aPath, sizeof(aPath), "editor/automap/%s.rules", pTileName); - IOHANDLE RulesFile = Storage()->OpenFile(aPath, IOFLAG_READ | IOFLAG_SKIP_BOM, IStorage::TYPE_ALL); - if(!RulesFile) + CLineReader LineReader; + if(!LineReader.OpenFile(Storage()->OpenFile(aPath, IOFLAG_READ, IStorage::TYPE_ALL))) { char aBuf[IO_MAX_PATH_LENGTH + 32]; str_format(aBuf, sizeof(aBuf), "failed to load %s", aPath); @@ -58,15 +58,12 @@ void CAutoMapper::Load(const char *pTileName) return; } - CLineReader LineReader; - LineReader.Init(RulesFile); - CConfiguration *pCurrentConf = nullptr; CRun *pCurrentRun = nullptr; CIndexRule *pCurrentIndex = nullptr; // read each line - while(char *pLine = LineReader.Get()) + while(const char *pLine = LineReader.Get()) { // skip blank/empty lines as well as comments if(str_length(pLine) > 0 && pLine[0] != '#' && pLine[0] != '\n' && pLine[0] != '\r' && pLine[0] != '\t' && pLine[0] != '\v' && pLine[0] != ' ') @@ -365,8 +362,6 @@ void CAutoMapper::Load(const char *pTileName) } } - io_close(RulesFile); - char aBuf[IO_MAX_PATH_LENGTH + 16]; str_format(aBuf, sizeof(aBuf), "loaded %s", aPath); Console()->Print(IConsole::OUTPUT_LEVEL_DEBUG, "editor/automap", aBuf); diff --git a/src/game/localization.cpp b/src/game/localization.cpp index 7d65b4426..5f29840f2 100644 --- a/src/game/localization.cpp +++ b/src/game/localization.cpp @@ -23,18 +23,14 @@ void CLocalizationDatabase::LoadIndexfile(IStorage *pStorage, IConsole *pConsole m_vLanguages.emplace_back("English", "", 826, vEnglishLanguageCodes); const char *pFilename = "languages/index.txt"; - IOHANDLE File = pStorage->OpenFile(pFilename, IOFLAG_READ | IOFLAG_SKIP_BOM, IStorage::TYPE_ALL); - if(!File) + CLineReader LineReader; + if(!LineReader.OpenFile(pStorage->OpenFile(pFilename, IOFLAG_READ, IStorage::TYPE_ALL))) { log_error("localization", "Couldn't open index file '%s'", pFilename); return; } - CLineReader LineReader; - LineReader.Init(File); - - const char *pLine; - while((pLine = LineReader.Get())) + while(const char *pLine = LineReader.Get()) { if(!str_length(pLine) || pLine[0] == '#') // skip empty lines and comments continue; @@ -105,8 +101,6 @@ void CLocalizationDatabase::LoadIndexfile(IStorage *pStorage, IConsole *pConsole m_vLanguages.emplace_back(aNativeName, aFileName, str_toint(aCountryCode), vLanguageCodes); } - io_close(File); - std::sort(m_vLanguages.begin(), m_vLanguages.end()); } @@ -179,8 +173,8 @@ bool CLocalizationDatabase::Load(const char *pFilename, IStorage *pStorage, ICon return true; } - IOHANDLE IoHandle = pStorage->OpenFile(pFilename, IOFLAG_READ | IOFLAG_SKIP_BOM, IStorage::TYPE_ALL); - if(!IoHandle) + CLineReader LineReader; + if(!LineReader.OpenFile(pStorage->OpenFile(pFilename, IOFLAG_READ, IStorage::TYPE_ALL))) return false; log_info("localization", "loaded '%s'", pFilename); @@ -189,11 +183,8 @@ bool CLocalizationDatabase::Load(const char *pFilename, IStorage *pStorage, ICon char aContext[512]; char aOrigin[512]; - CLineReader LineReader; - LineReader.Init(IoHandle); - char *pLine; int Line = 0; - while((pLine = LineReader.Get())) + while(const char *pLine = LineReader.Get()) { Line++; if(!str_length(pLine)) @@ -225,7 +216,7 @@ bool CLocalizationDatabase::Load(const char *pFilename, IStorage *pStorage, ICon } str_copy(aOrigin, pLine); - char *pReplacement = LineReader.Get(); + const char *pReplacement = LineReader.Get(); if(!pReplacement) { log_error("localization", "unexpected end of file after original '%s' on line %d", aOrigin, Line); @@ -242,7 +233,6 @@ bool CLocalizationDatabase::Load(const char *pFilename, IStorage *pStorage, ICon pReplacement += 3; AddString(aOrigin, pReplacement, aContext); } - io_close(IoHandle); std::sort(m_vStrings.begin(), m_vStrings.end()); return true; } diff --git a/src/game/server/gamecontext.cpp b/src/game/server/gamecontext.cpp index 0d1fae675..5a48cf252 100644 --- a/src/game/server/gamecontext.cpp +++ b/src/game/server/gamecontext.cpp @@ -3846,21 +3846,17 @@ void CGameContext::OnInit(const void *pPersistentData) m_pController = new CGameControllerDDRace(this); const char *pCensorFilename = "censorlist.txt"; - IOHANDLE File = Storage()->OpenFile(pCensorFilename, IOFLAG_READ | IOFLAG_SKIP_BOM, IStorage::TYPE_ALL); - if(!File) + CLineReader LineReader; + if(LineReader.OpenFile(Storage()->OpenFile(pCensorFilename, IOFLAG_READ, IStorage::TYPE_ALL))) { - dbg_msg("censorlist", "failed to open '%s'", pCensorFilename); - } - else - { - CLineReader LineReader; - LineReader.Init(File); - char *pLine; - while((pLine = LineReader.Get())) + while(const char *pLine = LineReader.Get()) { m_vCensorlist.emplace_back(pLine); } - io_close(File); + } + else + { + dbg_msg("censorlist", "failed to open '%s'", pCensorFilename); } m_TeeHistorianActive = g_Config.m_SvTeeHistorian; @@ -4064,36 +4060,28 @@ void CGameContext::OnMapChange(char *pNewMapName, int MapNameSize) char aConfig[IO_MAX_PATH_LENGTH]; str_format(aConfig, sizeof(aConfig), "maps/%s.cfg", g_Config.m_SvMap); - IOHANDLE File = Storage()->OpenFile(aConfig, IOFLAG_READ | IOFLAG_SKIP_BOM, IStorage::TYPE_ALL); - if(!File) + CLineReader LineReader; + if(!LineReader.OpenFile(Storage()->OpenFile(aConfig, IOFLAG_READ, IStorage::TYPE_ALL))) { // No map-specific config, just return. return; } - CLineReader LineReader; - LineReader.Init(File); - std::vector vLines; - char *pLine; + std::vector vpLines; int TotalLength = 0; - while((pLine = LineReader.Get())) + while(const char *pLine = LineReader.Get()) { - int Length = str_length(pLine) + 1; - char *pCopy = (char *)malloc(Length); - str_copy(pCopy, pLine, Length); - vLines.push_back(pCopy); - TotalLength += Length; + vpLines.push_back(pLine); + TotalLength += str_length(pLine) + 1; } - io_close(File); char *pSettings = (char *)malloc(maximum(1, TotalLength)); int Offset = 0; - for(auto &Line : vLines) + for(const char *pLine : vpLines) { - int Length = str_length(Line) + 1; - mem_copy(pSettings + Offset, Line, Length); + int Length = str_length(pLine) + 1; + mem_copy(pSettings + Offset, pLine, Length); Offset += Length; - free(Line); } CDataFileReader Reader; diff --git a/src/game/server/score.cpp b/src/game/server/score.cpp index ed149e8a4..60d0efc8c 100644 --- a/src/game/server/score.cpp +++ b/src/game/server/score.cpp @@ -79,20 +79,16 @@ CScore::CScore(CGameContext *pGameServer, CDbConnectionPool *pPool) : secure_random_fill(aSeed, sizeof(aSeed)); m_Prng.Seed(aSeed); - IOHANDLE File = GameServer()->Storage()->OpenFile("wordlist.txt", IOFLAG_READ | IOFLAG_SKIP_BOM, IStorage::TYPE_ALL); - if(File) + CLineReader LineReader; + if(LineReader.OpenFile(GameServer()->Storage()->OpenFile("wordlist.txt", IOFLAG_READ, IStorage::TYPE_ALL))) { - CLineReader LineReader; - LineReader.Init(File); - char *pLine; - while((pLine = LineReader.Get())) + while(const char *pLine = LineReader.Get()) { char aWord[32] = {0}; sscanf(pLine, "%*s %31s", aWord); aWord[31] = 0; m_vWordlist.emplace_back(aWord); } - io_close(File); } else { diff --git a/src/test/io.cpp b/src/test/io.cpp index e2e488bea..dde7c22ec 100644 --- a/src/test/io.cpp +++ b/src/test/io.cpp @@ -3,18 +3,19 @@ #include -void TestFileRead(const char *pWritten, bool SkipBom, const char *pRead) +void TestFileRead(const char *pWritten) { CTestInfo Info; char aBuf[512] = {0}; IOHANDLE File = io_open(Info.m_aFilename, IOFLAG_WRITE); + const int WrittenLength = str_length(pWritten); ASSERT_TRUE(File); - EXPECT_EQ(io_write(File, pWritten, str_length(pWritten)), str_length(pWritten)); + EXPECT_EQ(io_write(File, pWritten, WrittenLength), WrittenLength); EXPECT_FALSE(io_close(File)); - File = io_open(Info.m_aFilename, IOFLAG_READ | (SkipBom ? IOFLAG_SKIP_BOM : 0)); + File = io_open(Info.m_aFilename, IOFLAG_READ); ASSERT_TRUE(File); - EXPECT_EQ(io_read(File, aBuf, sizeof(aBuf)), str_length(pRead)); - EXPECT_TRUE(mem_comp(aBuf, pRead, str_length(pRead)) == 0); + EXPECT_EQ(io_read(File, aBuf, sizeof(aBuf)), WrittenLength); + EXPECT_TRUE(mem_comp(aBuf, pWritten, WrittenLength) == 0); EXPECT_FALSE(io_close(File)); fs_remove(Info.m_aFilename); @@ -22,45 +23,21 @@ void TestFileRead(const char *pWritten, bool SkipBom, const char *pRead) TEST(Io, Read1) { - TestFileRead("", false, ""); + TestFileRead(""); } TEST(Io, Read2) { - TestFileRead("abc", false, "abc"); + TestFileRead("abc"); } TEST(Io, Read3) { - TestFileRead("\xef\xbb\xbf", false, "\xef\xbb\xbf"); + TestFileRead("\xef\xbb\xbf"); } TEST(Io, Read4) { - TestFileRead("\xef\xbb\xbfxyz", false, "\xef\xbb\xbfxyz"); + TestFileRead("\xef\xbb\xbfxyz"); } -TEST(Io, ReadBom1) -{ - TestFileRead("", true, ""); -} -TEST(Io, ReadBom2) -{ - TestFileRead("abc", true, "abc"); -} -TEST(Io, ReadBom3) -{ - TestFileRead("\xef\xbb\xbf", true, ""); -} -TEST(Io, ReadBom4) -{ - TestFileRead("\xef\xbb\xbfxyz", true, "xyz"); -} -TEST(Io, ReadBom5) -{ - TestFileRead("\xef\xbb\xbf\xef\xbb\xbf", true, "\xef\xbb\xbf"); -} -TEST(Io, ReadBom6) -{ - TestFileRead("\xef\xbb\xbfxyz\xef\xbb\xbf", true, "xyz\xef\xbb\xbf"); -} TEST(Io, CurrentExe) { IOHANDLE CurrentExe = io_current_exe(); diff --git a/src/test/linereader.cpp b/src/test/linereader.cpp index 2127c4685..8efd83e1f 100644 --- a/src/test/linereader.cpp +++ b/src/test/linereader.cpp @@ -4,45 +4,89 @@ #include #include -void TestFileLineReader(const char *pWritten, bool SkipBom, std::initializer_list pReads) +void TestFileLineReaderRaw(const char *pWritten, unsigned WrittenLength, std::initializer_list pReads, bool ExpectSuccess, bool WriteBom) { CTestInfo Info; IOHANDLE File = io_open(Info.m_aFilename, IOFLAG_WRITE); ASSERT_TRUE(File); - EXPECT_EQ(io_write(File, pWritten, str_length(pWritten)), str_length(pWritten)); - EXPECT_FALSE(io_close(File)); - File = io_open(Info.m_aFilename, IOFLAG_READ | (SkipBom ? IOFLAG_SKIP_BOM : 0)); - ASSERT_TRUE(File); - CLineReader LineReader; - LineReader.Init(File); - for(const char *pRead : pReads) + if(WriteBom) { - const char *pReadLine = LineReader.Get(); - ASSERT_TRUE(pReadLine); - EXPECT_STREQ(pReadLine, pRead); + constexpr const unsigned char UTF8_BOM[] = {0xEF, 0xBB, 0xBF}; + EXPECT_EQ(io_write(File, UTF8_BOM, sizeof(UTF8_BOM)), sizeof(UTF8_BOM)); } - EXPECT_FALSE(LineReader.Get()); + EXPECT_EQ(io_write(File, pWritten, WrittenLength), WrittenLength); EXPECT_FALSE(io_close(File)); + CLineReader LineReader; + const bool ActualSuccess = LineReader.OpenFile(io_open(Info.m_aFilename, IOFLAG_READ)); + ASSERT_EQ(ActualSuccess, ExpectSuccess); + if(ActualSuccess) + { + for(const char *pRead : pReads) + { + const char *pReadLine = LineReader.Get(); + ASSERT_TRUE(pReadLine) << "Line reader returned less lines than expected"; + EXPECT_STREQ(pReadLine, pRead) << "Line reader returned unexpected line"; + } + EXPECT_FALSE(LineReader.Get()) << "Line reader returned more lines than expected"; + } + fs_remove(Info.m_aFilename); } +void TestFileLineReaderRaw(const char *pWritten, unsigned WrittenLength, std::initializer_list pReads, bool ExpectSuccess) +{ + TestFileLineReaderRaw(pWritten, WrittenLength, pReads, ExpectSuccess, false); + TestFileLineReaderRaw(pWritten, WrittenLength, pReads, ExpectSuccess, true); +} + +void TestFileLineReader(const char *pWritten, std::initializer_list pReads) +{ + TestFileLineReaderRaw(pWritten, str_length(pWritten), pReads, true); +} + TEST(LineReader, NormalNewline) { - TestFileLineReader("foo\nbar\nbaz\n", false, {"foo", "bar", "baz"}); + TestFileLineReader("foo\nbar\nbaz", {"foo", "bar", "baz"}); + TestFileLineReader("foo\nbar\nbaz\n", {"foo", "bar", "baz"}); } TEST(LineReader, CRLFNewline) { - TestFileLineReader("foo\r\nbar\r\nbaz", true, {"foo", "bar", "baz"}); + TestFileLineReader("foo\r\nbar\r\nbaz", {"foo", "bar", "baz"}); + TestFileLineReader("foo\r\nbar\r\nbaz\r\n", {"foo", "bar", "baz"}); +} + +TEST(LineReader, MixedNewline) +{ + TestFileLineReader("1\n2\r\n3\n4\n5\r\n6", {"1", "2", "3", "4", "5", "6"}); + TestFileLineReader("1\n2\r\n3\n4\n5\r\n6\n", {"1", "2", "3", "4", "5", "6"}); + TestFileLineReader("1\n2\r\n3\n4\n5\r\n6\r\n", {"1", "2", "3", "4", "5", "6"}); + TestFileLineReader("1\n2\r\n3\n4\n5\r\n6\r", {"1", "2", "3", "4", "5", "6\r"}); +} + +TEST(LineReader, EmptyLines) +{ + TestFileLineReader("\n\r\n\n\n\r\n", {"", "", "", "", ""}); + TestFileLineReader("\n\r\n\n\n\r\n\n", {"", "", "", "", "", ""}); + TestFileLineReader("\n\r\n\n\n\r\n\r\n", {"", "", "", "", "", ""}); + TestFileLineReader("\n\r\n\n\n\r\n\r", {"", "", "", "", "", "\r"}); } TEST(LineReader, Invalid) { // Lines containing invalid UTF-8 are skipped - TestFileLineReader("foo\xff\nbar\xff\nbaz\xff\n", false, {}); - TestFileLineReader("foo\xff\nbar\nbaz\n", false, {"bar", "baz"}); - TestFileLineReader("foo\nbar\xff\nbaz\n", false, {"foo", "baz"}); - TestFileLineReader("foo\nbar\nbaz\xff\n", false, {"foo", "bar"}); - TestFileLineReader("foo\nbar1\xff\nbar2\xff\nfoobar\nbar3\xff\nbaz\n", false, {"foo", "foobar", "baz"}); + TestFileLineReader("foo\xff\nbar\xff\nbaz\xff", {}); + TestFileLineReader("foo\xff\nbar\nbaz", {"bar", "baz"}); + TestFileLineReader("foo\nbar\xff\nbaz", {"foo", "baz"}); + TestFileLineReader("foo\nbar\nbaz\xff", {"foo", "bar"}); + TestFileLineReader("foo\nbar1\xff\nbar2\xff\nfoobar\nbar3\xff\nbaz", {"foo", "foobar", "baz"}); +} + +TEST(LineReader, NullBytes) +{ + // Line reader does not read any lines if the file contains null bytes + TestFileLineReaderRaw("foo\0\nbar\nbaz", 12, {}, false); + TestFileLineReaderRaw("foo\nbar\0\nbaz", 12, {}, false); + TestFileLineReaderRaw("foo\nbar\nbaz\0", 12, {}, false); } diff --git a/src/tools/config_store.cpp b/src/tools/config_store.cpp index dd8fe05a4..f173f6f01 100644 --- a/src/tools/config_store.cpp +++ b/src/tools/config_store.cpp @@ -8,37 +8,28 @@ void Process(IStorage *pStorage, const char *pMapName, const char *pConfigName) { - IOHANDLE File = pStorage->OpenFile(pConfigName, IOFLAG_READ | IOFLAG_SKIP_BOM, IStorage::TYPE_ABSOLUTE); - if(!File) + CLineReader LineReader; + if(!LineReader.OpenFile(pStorage->OpenFile(pConfigName, IOFLAG_READ, IStorage::TYPE_ABSOLUTE))) { dbg_msg("config_store", "config '%s' not found", pConfigName); return; } - CLineReader LineReader; - LineReader.Init(File); - - char *pLine; + std::vector vpLines; int TotalLength = 0; - std::vector vLines; - while((pLine = LineReader.Get())) + while(const char *pLine = LineReader.Get()) { - int Length = str_length(pLine) + 1; - char *pCopy = (char *)malloc(Length); - mem_copy(pCopy, pLine, Length); - vLines.push_back(pCopy); - TotalLength += Length; + vpLines.push_back(pLine); + TotalLength += str_length(pLine) + 1; } - io_close(File); char *pSettings = (char *)malloc(maximum(1, TotalLength)); int Offset = 0; - for(auto &Line : vLines) + for(const char *pLine : vpLines) { - int Length = str_length(Line) + 1; - mem_copy(pSettings + Offset, Line, Length); + int Length = str_length(pLine) + 1; + mem_copy(pSettings + Offset, pLine, Length); Offset += Length; - free(Line); } CDataFileReader Reader;