Merge pull request #68 from lioncash/insensitive

CStringExtras: Prevent potential out of bounds reads with CompareCaseInsensitive

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)

Runtime/CPakFile.cpp

@@ -18,9 +18,11 @@ CPakFile::~CPakFile() {
 }
 
 const SObjectTag* CPakFile::GetResIdByName(std::string_view name) const {
-  for (const std::pair<std::string, SObjectTag>& p : x54_nameList)
+  for (const std::pair<std::string, SObjectTag>& p : x54_nameList) {
-    if (!CStringExtras::CompareCaseInsensitive(p.first.c_str(), name))
+    if (CStringExtras::CompareCaseInsensitive(p.first, name)) {
       return &p.second;
+    }
+  }
   return nullptr;
 }
 

Runtime/CResLoader.cpp

@@ -9,7 +9,7 @@ CResLoader::CResLoader() { x48_curPak = x18_pakLoadedList.end(); }
 const std::vector<CAssetId>* CResLoader::GetTagListForFile(std::string_view name) const {
   const std::string namePak = std::string(name).append(".upak");
   for (const std::unique_ptr<CPakFile>& 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<u8[]> CResLoader::LoadNewResourcePartSync(const urde::SObjectTag
 void CResLoader::GetTagListForFile(const char* pakName, std::vector<SObjectTag>& out) const {
   std::string path = std::string(pakName) + ".upak";
   for (const std::unique_ptr<CPakFile>& 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) {

Runtime/CStringExtras.hpp

@@ -1,29 +1,40 @@
 #pragma once
 
+#include <algorithm>
+#include <cctype>
 #include <string>
-#include <cstring>
 
 namespace urde {
 
 class CStringExtras {
 public:
-  static int CompareCaseInsensitive(const char* a, const char* b) {
+  // Checks if the provided views into string data can be considered equal or not based on
-#if _WIN32
+  // whether or not all their characters are lexicographically equal to one another in
-    return _stricmp(a, b);
+  // a character insensitive manner.
-#else
+  //
-    return strcasecmp(a, b);
+  // NOTE: This differs slightly from the actual version of this function within the game executable
-#endif
+  //       in order to better accomodate string views and potentially non-null-terminated string data.
-  }
+  //
-  static int CompareCaseInsensitive(std::string_view a, std::string_view b) {
+  //       In the game executable, the function essentially behaves like strcasecmp in that it returns
-    return CompareCaseInsensitive(a.data(), b.data());
+  //       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<unsigned char>(lhs)) < std::tolower(static_cast<unsigned char>(rhs));
+    });
   }
 
   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::transform(str.begin(), str.end(), str.begin(),
-    std::string::size_type s = str.find(needle);
+                   [](char c) { return std::tolower(static_cast<unsigned char>(c)); });
-    if (s == std::string::npos)
+    const std::string::size_type s = str.find(needle);
+    if (s == std::string::npos) {
       return -1;
+    }
     return s;
   }
 };

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;
 }