From 0d2b2f45a8200062faad5b891bc5c5a48fafdec3 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Sat, 14 Sep 2019 11:14:51 -0400 Subject: [PATCH 1/2] CStringExtras: Prevent potential out of bounds reads with CompareCaseInsensitive The strcasecmp and _stricmp functions expect the passed in strings to be null-terminated, however we we're also exposing a std::string_view overload for that function. std::string_view instances aren't required to be null-terminated, so this makes the interface a little unsafe. We can use std::lexicographical_compare() to provide the same behavior and also properly handle the case of non-null-terminated strings. --- Runtime/Audio/CStreamAudioManager.cpp | 6 +++--- Runtime/CPakFile.cpp | 6 ++++-- Runtime/CResLoader.cpp | 4 ++-- Runtime/CStringExtras.hpp | 29 +++++++++++++++++--------- Runtime/World/CScriptStreamedMusic.cpp | 3 ++- 5 files changed, 30 insertions(+), 18 deletions(-) diff --git a/Runtime/Audio/CStreamAudioManager.cpp b/Runtime/Audio/CStreamAudioManager.cpp index 6bb845958..dce6a7e4a 100644 --- a/Runtime/Audio/CStreamAudioManager.cpp +++ b/Runtime/Audio/CStreamAudioManager.cpp @@ -862,7 +862,7 @@ void CStreamAudioManager::Start(bool oneshot, std::string_view fileName, float v SDSPPlayer& p = s_Players[oneshot]; SDSPPlayer& qp = s_QueuedPlayers[oneshot]; - if (p.x10_playState != EPlayerState::Stopped && CStringExtras::CompareCaseInsensitive(fileName, p.x0_fileName)) { + if (p.x10_playState != EPlayerState::Stopped && !CStringExtras::CompareCaseInsensitive(fileName, p.x0_fileName)) { /* Enque new stream */ qp = SDSPPlayer(EPlayerState::FadeIn, fileName, volume, fadeIn, fadeOut, -1, music); Stop(oneshot, p.x0_fileName); @@ -900,10 +900,10 @@ void CStreamAudioManager::Stop(bool oneshot, std::string_view fileName) { SDSPPlayer& p = s_Players[oneshot]; SDSPPlayer& qp = s_QueuedPlayers[oneshot]; - if (!CStringExtras::CompareCaseInsensitive(fileName, qp.x0_fileName)) { + if (CStringExtras::CompareCaseInsensitive(fileName, qp.x0_fileName)) { /* Cancel enqueued file */ qp = SDSPPlayer(); - } else if (!CStringExtras::CompareCaseInsensitive(fileName, p.x0_fileName) && p.x20_internalHandle != -1 && + } else if (CStringExtras::CompareCaseInsensitive(fileName, p.x0_fileName) && p.x20_internalHandle != -1 && p.x10_playState != EPlayerState::Stopped) { /* Fade out or stop */ if (p.x1c_fadeOut <= FLT_EPSILON) diff --git a/Runtime/CPakFile.cpp b/Runtime/CPakFile.cpp index bb81bc631..4c6c25494 100644 --- a/Runtime/CPakFile.cpp +++ b/Runtime/CPakFile.cpp @@ -18,9 +18,11 @@ CPakFile::~CPakFile() { } const SObjectTag* CPakFile::GetResIdByName(std::string_view name) const { - for (const std::pair& p : x54_nameList) - if (!CStringExtras::CompareCaseInsensitive(p.first.c_str(), name)) + for (const std::pair& p : x54_nameList) { + if (CStringExtras::CompareCaseInsensitive(p.first, name)) { return &p.second; + } + } return nullptr; } diff --git a/Runtime/CResLoader.cpp b/Runtime/CResLoader.cpp index a100095c0..df607e59f 100644 --- a/Runtime/CResLoader.cpp +++ b/Runtime/CResLoader.cpp @@ -9,7 +9,7 @@ CResLoader::CResLoader() { x48_curPak = x18_pakLoadedList.end(); } const std::vector* CResLoader::GetTagListForFile(std::string_view name) const { const std::string namePak = std::string(name).append(".upak"); for (const std::unique_ptr& pak : x18_pakLoadedList) { - if (!CStringExtras::CompareCaseInsensitive(namePak, pak->x18_path)) { + if (CStringExtras::CompareCaseInsensitive(namePak, pak->x18_path)) { return &pak->GetDepList(); } } @@ -117,7 +117,7 @@ std::unique_ptr CResLoader::LoadNewResourcePartSync(const urde::SObjectTag void CResLoader::GetTagListForFile(const char* pakName, std::vector& out) const { std::string path = std::string(pakName) + ".upak"; for (const std::unique_ptr& file : x18_pakLoadedList) { - if (!CStringExtras::CompareCaseInsensitive(file->GetPath(), path)) { + if (CStringExtras::CompareCaseInsensitive(file->GetPath(), path)) { auto& depList = file->GetDepList(); out.reserve(depList.size()); for (const auto& dep : depList) { diff --git a/Runtime/CStringExtras.hpp b/Runtime/CStringExtras.hpp index 2c7d30363..399030d82 100644 --- a/Runtime/CStringExtras.hpp +++ b/Runtime/CStringExtras.hpp @@ -1,21 +1,30 @@ #pragma once +#include +#include #include -#include namespace urde { class CStringExtras { public: - static int CompareCaseInsensitive(const char* a, const char* b) { -#if _WIN32 - return _stricmp(a, b); -#else - return strcasecmp(a, b); -#endif - } - static int CompareCaseInsensitive(std::string_view a, std::string_view b) { - return CompareCaseInsensitive(a.data(), b.data()); + // Checks if the provided views into string data can be considered equal or not based on + // whether or not all their characters are lexicographically equal to one another in + // a character insensitive manner. + // + // NOTE: This differs slightly from the actual version of this function within the game executable + // in order to better accomodate string views and potentially non-null-terminated string data. + // + // In the game executable, the function essentially behaves like strcasecmp in that it returns + // an int indicating whether or not the first argument is lexicographically less than, equal to, + // or greater than the second argument. Given no usages in the code depend on the less than or + // greater than cases, but rather just care about whether or not the strings are equal to one + // another, this is a safe change to make. + // + static bool CompareCaseInsensitive(std::string_view a, std::string_view b) { + return std::lexicographical_compare(a.begin(), a.end(), b.begin(), b.end(), [](char lhs, char rhs) { + return std::tolower(static_cast(lhs)) < std::tolower(static_cast(rhs)); + }); } static int IndexOfSubstring(std::string_view haystack, std::string_view needle) { diff --git a/Runtime/World/CScriptStreamedMusic.cpp b/Runtime/World/CScriptStreamedMusic.cpp index 4df2cfe4f..4f7c03bbe 100644 --- a/Runtime/World/CScriptStreamedMusic.cpp +++ b/Runtime/World/CScriptStreamedMusic.cpp @@ -10,8 +10,9 @@ namespace urde { bool CScriptStreamedMusic::IsDSPFile(std::string_view fileName) { - if (!CStringExtras::CompareCaseInsensitive(fileName, "sw")) + if (CStringExtras::CompareCaseInsensitive(fileName, "sw")) { return true; + } return CStringExtras::IndexOfSubstring(fileName, ".dsp") != -1; } From 991d04869421828cf6a08ff40af54e3cfbcc45c4 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Sat, 14 Sep 2019 11:28:15 -0400 Subject: [PATCH 2/2] CStringExtras: Prevent undefined behavior within IndexOfSubstring Unlikely to occur, but does completely prevent the case of undefined behavior if a non-ascii character ends up within the given string. --- Runtime/CStringExtras.hpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/Runtime/CStringExtras.hpp b/Runtime/CStringExtras.hpp index 399030d82..bd022ce09 100644 --- a/Runtime/CStringExtras.hpp +++ b/Runtime/CStringExtras.hpp @@ -29,10 +29,12 @@ public: static int IndexOfSubstring(std::string_view haystack, std::string_view needle) { std::string str(haystack); - std::transform(str.begin(), str.end(), str.begin(), tolower); - std::string::size_type s = str.find(needle); - if (s == std::string::npos) + std::transform(str.begin(), str.end(), str.begin(), + [](char c) { return std::tolower(static_cast(c)); }); + const std::string::size_type s = str.find(needle); + if (s == std::string::npos) { return -1; + } return s; } };