From 406cb049520bf324975f698218562458f1095a21 Mon Sep 17 00:00:00 2001 From: Christoph Oelckers Date: Sat, 6 Jan 2024 12:25:16 +0100 Subject: [PATCH] address a few more issues found by static analysis * mark move constructors and operators noexcept. * eliminate large stack allocations in several places. * some incorrect checks for Windows handles. --- CMakeLists.txt | 7 +++++++ source/common/audio/music/i_soundfont.cpp | 13 +++++-------- source/common/audio/music/music.cpp | 8 ++++---- source/common/filesystem/source/ancientzip.cpp | 16 +++++++++++++--- source/common/platform/win32/i_crash.cpp | 12 +++++++----- source/common/platform/win32/i_dijoy.cpp | 3 ++- source/common/platform/win32/i_input.cpp | 14 +++++++++----- source/common/platform/win32/i_main.cpp | 4 ++-- source/common/rendering/gl_load/gl_load.c | 17 +++++++++++++---- source/common/textures/hires/hqresize.cpp | 4 +++- source/common/textures/m_png.cpp | 6 ++++-- source/common/thirdparty/gain_analysis.cpp | 2 +- 12 files changed, 70 insertions(+), 36 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index a08678130..25b3843bb 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -299,6 +299,13 @@ if( MSVC ) # These must be silenced because the code is full of them. Expect some of undefined behavior hidden in this mess. :( #set( ALL_C_FLAGS "${ALL_C_FLAGS} /wd4244 /wd4018 /wd4267" ) + # annoying intellisense warnings that are virtually always false positives and mostly happen in third party code. + #6297: overflow when shifting + #6386: Buffer overrun when writing (i.e. missing range check for array writes) + #6385: Reading invalid data (i.e. missing range check for array reads) + #6308: missing null check on realloc + set( ALL_C_FLAGS "${ALL_C_FLAGS} /wd6297 /wd6386 /wd6385 /wd6308" ) + # Most of these need to be cleaned out. The source is currently infested with far too much conditional compilation which is a constant source of problems. set( ALL_C_FLAGS "${ALL_C_FLAGS} /DUSE_OPENGL=1 /DNOASM=1 /DWIN32" ) diff --git a/source/common/audio/music/i_soundfont.cpp b/source/common/audio/music/i_soundfont.cpp index 0f3c0020b..9737374fc 100644 --- a/source/common/audio/music/i_soundfont.cpp +++ b/source/common/audio/music/i_soundfont.cpp @@ -465,19 +465,16 @@ const FSoundFontInfo *FSoundFontManager::FindSoundFont(const char *name, int all // //========================================================================== -FSoundFontReader *FSoundFontManager::OpenSoundFont(const char *name, int allowed) +FSoundFontReader *FSoundFontManager::OpenSoundFont(const char *const name, int allowed) { - + if (name == nullptr) return nullptr; // First check if the given name is inside the loaded resources. // To avoid clashes this will only be done if the name has the '.cfg' extension. // Sound fonts cannot be loaded this way. - if (name != nullptr) + const char *p = name + strlen(name) - 4; + if (p > name && !stricmp(p, ".cfg") && fileSystem.CheckNumForFullName(name) >= 0) { - const char *p = name + strlen(name) - 4; - if (p > name && !stricmp(p, ".cfg") && fileSystem.CheckNumForFullName(name) >= 0) - { - return new FLumpPatchSetReader(name); - } + return new FLumpPatchSetReader(name); } // Next check if the file is a .sf file diff --git a/source/common/audio/music/music.cpp b/source/common/audio/music/music.cpp index 769d5c07c..13837e93e 100644 --- a/source/common/audio/music/music.cpp +++ b/source/common/audio/music/music.cpp @@ -432,14 +432,14 @@ static FString ReplayGainHash(ZMusicCustomReader* reader, int flength, int playe { std::string playparam = _playparam; - uint8_t buffer[50000]; // for performance reasons only hash the start of the file. If we wanted to do this to large waveform songs it'd cause noticable lag. + TArray buffer(50000, true); // for performance reasons only hash the start of the file. If we wanted to do this to large waveform songs it'd cause noticable lag. uint8_t digest[16]; char digestout[33]; - auto length = reader->read(reader, buffer, 50000); + auto length = reader->read(reader, buffer.data(), 50000); reader->seek(reader, 0, SEEK_SET); MD5Context md5; md5.Init(); - md5.Update(buffer, (int)length); + md5.Update(buffer.data(), (int)length); md5.Final(digest); for (size_t j = 0; j < sizeof(digest); ++j) @@ -448,7 +448,7 @@ static FString ReplayGainHash(ZMusicCustomReader* reader, int flength, int playe } digestout[32] = 0; - auto type = ZMusic_IdentifyMIDIType((uint32_t*)buffer, 32); + auto type = ZMusic_IdentifyMIDIType((uint32_t*)buffer.data(), 32); if (type == MIDI_NOTMIDI) return FStringf("%d:%s", flength, digestout); // get the default for MIDI synth diff --git a/source/common/filesystem/source/ancientzip.cpp b/source/common/filesystem/source/ancientzip.cpp index 34356306e..9f5ebdfef 100644 --- a/source/common/filesystem/source/ancientzip.cpp +++ b/source/common/filesystem/source/ancientzip.cpp @@ -45,6 +45,7 @@ #include #include +#include #include "ancientzip.h" namespace FileSys { @@ -342,10 +343,19 @@ int FZipExploder::Explode(unsigned char *out, unsigned int outsize, int ShrinkLoop(unsigned char *out, unsigned int outsize, FileReader &_In, unsigned int InLeft) { + // don't allocate this on the stack, it's a bit on the large side. + struct work + { + unsigned char ReadBuf[256]; + unsigned short Parent[HSIZE]; + unsigned char Value[HSIZE], Stack[HSIZE]; + }; + auto s = std::make_unique(); + unsigned char* ReadBuf = s->ReadBuf; + unsigned short* Parent = s->Parent; + unsigned char* Value = s->Value, * Stack = s->Stack; + FileReader *In = &_In; - unsigned char ReadBuf[256]; - unsigned short Parent[HSIZE]; - unsigned char Value[HSIZE], Stack[HSIZE]; unsigned char *newstr; int len; int KwKwK, codesize = 9; /* start at 9 bits/code */ diff --git a/source/common/platform/win32/i_crash.cpp b/source/common/platform/win32/i_crash.cpp index ab65b2ea5..dd5869281 100644 --- a/source/common/platform/win32/i_crash.cpp +++ b/source/common/platform/win32/i_crash.cpp @@ -381,12 +381,14 @@ static HANDLE WriteMyMiniDump (void) { MiniDumpThreadData dumpdata = { file, pMiniDumpWriteDump, &exceptor }; DWORD id; - HANDLE thread = CreateThread (NULL, 0, WriteMiniDumpInAnotherThread, - &dumpdata, 0, &id); - WaitForSingleObject (thread, INFINITE); - if (GetExitCodeThread (thread, &id)) + HANDLE thread = CreateThread (NULL, 0, WriteMiniDumpInAnotherThread, &dumpdata, 0, &id); + if (thread != nullptr) { - good = id; + WaitForSingleObject(thread, INFINITE); + if (GetExitCodeThread(thread, &id)) + { + good = id; + } } } } diff --git a/source/common/platform/win32/i_dijoy.cpp b/source/common/platform/win32/i_dijoy.cpp index a81fa1a26..492cbda14 100644 --- a/source/common/platform/win32/i_dijoy.cpp +++ b/source/common/platform/win32/i_dijoy.cpp @@ -421,7 +421,8 @@ void FDInputJoystick::ProcessInput() return; } - state = (uint8_t *)alloca(DataFormat.dwDataSize); + TArray statearr(DataFormat.dwDataSize, true); + state = statearr.data(); hr = Device->GetDeviceState(DataFormat.dwDataSize, state); if (FAILED(hr)) return; diff --git a/source/common/platform/win32/i_input.cpp b/source/common/platform/win32/i_input.cpp index 4ce8766aa..054c99590 100644 --- a/source/common/platform/win32/i_input.cpp +++ b/source/common/platform/win32/i_input.cpp @@ -334,7 +334,8 @@ LRESULT CALLBACK WndProc (HWND hWnd, UINT message, WPARAM wParam, LPARAM lParam) if (!GetRawInputData((HRAWINPUT)lParam, RID_INPUT, NULL, &size, sizeof(RAWINPUTHEADER)) && size != 0) { - uint8_t *buffer = (uint8_t *)alloca(size); + TArray array(size, true); + uint8_t *buffer = array.data(); if (GetRawInputData((HRAWINPUT)lParam, RID_INPUT, buffer, &size, sizeof(RAWINPUTHEADER)) == size) { int code = GET_RAWINPUT_CODE_WPARAM(wParam); @@ -691,12 +692,15 @@ void I_PutInClipboard (const char *str) auto wstr = WideString(str); HGLOBAL cliphandle = GlobalAlloc (GMEM_DDESHARE, wstr.length() * 2 + 2); - if (cliphandle != NULL) + if (cliphandle != nullptr) { wchar_t *ptr = (wchar_t *)GlobalLock (cliphandle); - wcscpy (ptr, wstr.c_str()); - GlobalUnlock (cliphandle); - SetClipboardData (CF_UNICODETEXT, cliphandle); + if (ptr) + { + wcscpy(ptr, wstr.c_str()); + GlobalUnlock(cliphandle); + SetClipboardData(CF_UNICODETEXT, cliphandle); + } } CloseClipboard (); } diff --git a/source/common/platform/win32/i_main.cpp b/source/common/platform/win32/i_main.cpp index d1f82e59a..6977055b7 100644 --- a/source/common/platform/win32/i_main.cpp +++ b/source/common/platform/win32/i_main.cpp @@ -284,12 +284,12 @@ int DoMain (HINSTANCE hInstance) HANDLE stdinput = GetStdHandle(STD_INPUT_HANDLE); ShowWindow(mainwindow.GetHandle(), SW_HIDE); - WriteFile(StdOut, "Press any key to exit...", 24, &bytes, NULL); + if (StdOut != nullptr) WriteFile(StdOut, "Press any key to exit...", 24, &bytes, nullptr); FlushConsoleInputBuffer(stdinput); SetConsoleMode(stdinput, 0); ReadConsole(stdinput, &bytes, 1, &bytes, NULL); } - else if (StdOut == NULL) + else if (StdOut == nullptr) { mainwindow.ShowErrorPane(nullptr); } diff --git a/source/common/rendering/gl_load/gl_load.c b/source/common/rendering/gl_load/gl_load.c index e29207f4a..14aba7a70 100644 --- a/source/common/rendering/gl_load/gl_load.c +++ b/source/common/rendering/gl_load/gl_load.c @@ -77,10 +77,19 @@ static void CheckOpenGL(void) if (opengl32dll == 0) { opengl32dll = LoadLibrary(L"OpenGL32.DLL"); - createcontext = (HGLRC(WINAPI*)(HDC)) GetProcAddress(opengl32dll, "wglCreateContext"); - deletecontext = (BOOL(WINAPI*)(HGLRC)) GetProcAddress(opengl32dll, "wglDeleteContext"); - makecurrent = (BOOL(WINAPI*)(HDC, HGLRC)) GetProcAddress(opengl32dll, "wglMakeCurrent"); - getprocaddress = (PROC(WINAPI*)(LPCSTR)) GetProcAddress(opengl32dll, "wglGetProcAddress"); + if (opengl32dll != 0) + { + createcontext = (HGLRC(WINAPI*)(HDC)) GetProcAddress(opengl32dll, "wglCreateContext"); + deletecontext = (BOOL(WINAPI*)(HGLRC)) GetProcAddress(opengl32dll, "wglDeleteContext"); + makecurrent = (BOOL(WINAPI*)(HDC, HGLRC)) GetProcAddress(opengl32dll, "wglMakeCurrent"); + getprocaddress = (PROC(WINAPI*)(LPCSTR)) GetProcAddress(opengl32dll, "wglGetProcAddress"); + } + else + { + // Should this ever happen we have no choice but to hard abort, there is no good way to recover. + MessageBoxA(0, "OpenGL32.dll not found", "Fatal error", MB_OK | MB_ICONERROR | MB_TASKMODAL); + exit(3); + } } } diff --git a/source/common/textures/hires/hqresize.cpp b/source/common/textures/hires/hqresize.cpp index 9a6c42aa2..fc3f13a16 100644 --- a/source/common/textures/hires/hqresize.cpp +++ b/source/common/textures/hires/hqresize.cpp @@ -39,6 +39,7 @@ #ifdef HAVE_MMX #include "hqnx_asm/hqnx_asm.h" #endif +#include #include "xbr/xbrz.h" #include "xbr/xbrz_old.h" #include "parallel_for.h" @@ -304,7 +305,8 @@ static unsigned char *hqNxAsmHelper( void (*hqNxFunction) ( int*, unsigned char* initdone = true; } - HQnX_asm::CImage cImageIn; + auto pImageIn = std::make_unique(); + auto& cImageIn = *pImageIn; cImageIn.SetImage(inputBuffer, inWidth, inHeight, 32); cImageIn.Convert32To17(); diff --git a/source/common/textures/m_png.cpp b/source/common/textures/m_png.cpp index 43e548e6c..331b35e20 100644 --- a/source/common/textures/m_png.cpp +++ b/source/common/textures/m_png.cpp @@ -510,7 +510,8 @@ bool M_ReadIDAT (FileReader &file, uint8_t *buffer, int width, int height, int p { i += bytesPerRowOut * 2; } - inputLine = (Byte *)alloca (i); + TArray inputArray(i, true); + inputLine = inputArray.data(); adam7buff[0] = inputLine + 4 + bytesPerRowOut; adam7buff[1] = adam7buff[0] + bytesPerRowOut; adam7buff[2] = adam7buff[1] + bytesPerRowOut; @@ -925,7 +926,8 @@ bool M_SaveBitmap(const uint8_t *from, ESSType color_type, int width, int height temprow[i] = &temprow_storage[temprow_size * i]; } - Byte buffer[PNG_WRITE_SIZE]; + TArray array(PNG_WRITE_SIZE, true); + auto buffer = array.data(); z_stream stream; int err; int y; diff --git a/source/common/thirdparty/gain_analysis.cpp b/source/common/thirdparty/gain_analysis.cpp index 2f92e2128..a31538f40 100644 --- a/source/common/thirdparty/gain_analysis.cpp +++ b/source/common/thirdparty/gain_analysis.cpp @@ -282,7 +282,7 @@ GainAnalyzer::ResetSampleFrequency(int samplefreq) { int GainAnalyzer::InitGainAnalysis(int samplefreq) { - *this = {}; + memset(this, 0, sizeof(*this)); if (ResetSampleFrequency(samplefreq) != INIT_GAIN_ANALYSIS_OK) { return INIT_GAIN_ANALYSIS_ERROR; }