From dae44c1120f1a26715adb47a6aca316aadaeb527 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Wed, 24 Jan 2024 09:23:41 +0100 Subject: [PATCH 1/6] SavedataParam: optimize PSPMatch --- Core/Dialog/SavedataParam.cpp | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/Core/Dialog/SavedataParam.cpp b/Core/Dialog/SavedataParam.cpp index fd0477f572..6d8427607a 100644 --- a/Core/Dialog/SavedataParam.cpp +++ b/Core/Dialog/SavedataParam.cpp @@ -110,32 +110,31 @@ namespace return info; } - bool PSPMatch(const std::string &text, const std::string ®exp) - { - if(text.empty() && regexp.empty()) + bool PSPMatch(std::string_view text, std::string_view regexp) { + if (text.empty() && regexp.empty()) return true; - else if(regexp == "*") + else if (regexp == "*") return true; - else if(text.empty()) + else if (text.empty()) return false; - else if(regexp.empty()) + else if (regexp.empty()) return false; - else if(regexp == "?" && text.length() == 1) + else if (regexp == "?" && text.length() == 1) return true; - else if(text == regexp) + else if (text == regexp) return true; - else if(regexp.data()[0] == '*') + else if (regexp.data()[0] == '*') { bool res = PSPMatch(text.substr(1),regexp.substr(1)); if(!res) res = PSPMatch(text.substr(1),regexp); return res; } - else if(regexp.data()[0] == '?') + else if (regexp.data()[0] == '?') { return PSPMatch(text.substr(1),regexp.substr(1)); } - else if(regexp.data()[0] == text.data()[0]) + else if (regexp.data()[0] == text.data()[0]) { return PSPMatch(text.substr(1),regexp.substr(1)); } From e5178d2bc405471c8f905f2ecb2a5df9b4906b8c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Sun, 5 May 2024 11:58:35 +0200 Subject: [PATCH 2/6] Remove unused code, some string_view --- Common/Data/Format/IniFile.h | 32 +------------------------------- Common/StringUtils.cpp | 8 ++++---- Common/StringUtils.h | 2 +- 3 files changed, 6 insertions(+), 36 deletions(-) diff --git a/Common/Data/Format/IniFile.h b/Common/Data/Format/IniFile.h index 8fa1e5c750..d5161787fe 100644 --- a/Common/Data/Format/IniFile.h +++ b/Common/Data/Format/IniFile.h @@ -133,30 +133,7 @@ public: // Returns true if key exists in section bool Exists(const char* sectionName, const char* key) const; - // TODO: Get rid of these, in favor of the Section ones. - void Set(const char* sectionName, const char* key, const char* newValue) { - GetOrCreateSection(sectionName)->Set(key, newValue); - } - void Set(const char* sectionName, const char* key, const std::string& newValue) { - GetOrCreateSection(sectionName)->Set(key, newValue.c_str()); - } - void Set(const char* sectionName, const char* key, int newValue) { - GetOrCreateSection(sectionName)->Set(key, newValue); - } - void Set(const char* sectionName, const char* key, uint32_t newValue) { - GetOrCreateSection(sectionName)->Set(key, newValue); - } - void Set(const char* sectionName, const char* key, uint64_t newValue) { - GetOrCreateSection(sectionName)->Set(key, newValue); - } - void Set(const char* sectionName, const char* key, bool newValue) { - GetOrCreateSection(sectionName)->Set(key, newValue); - } - void Set(const char* sectionName, const char* key, const std::vector& newValues) { - GetOrCreateSection(sectionName)->Set(key, newValues); - } - - // TODO: Get rid of these, in favor of the Section ones. + // These will not create the section if it doesn't exist. bool Get(const char* sectionName, const char* key, std::string* value, const char* defaultValue = ""); bool Get(const char* sectionName, const char* key, int* value, int defaultValue = 0); bool Get(const char* sectionName, const char* key, uint32_t* value, uint32_t defaultValue = 0); @@ -164,13 +141,6 @@ public: bool Get(const char* sectionName, const char* key, bool* value, bool defaultValue = false); bool Get(const char* sectionName, const char* key, std::vector& values); - template bool GetIfExists(const char* sectionName, const char* key, T value) - { - if (Exists(sectionName, key)) - return Get(sectionName, key, value); - return false; - } - bool GetKeys(const char* sectionName, std::vector& keys) const; bool DeleteKey(const char* sectionName, const char* key); diff --git a/Common/StringUtils.cpp b/Common/StringUtils.cpp index abf2970ce5..7aa9c55945 100644 --- a/Common/StringUtils.cpp +++ b/Common/StringUtils.cpp @@ -334,7 +334,7 @@ void SplitString(std::string_view str, const char delim, std::vector &output) { +void GetQuotedStrings(std::string_view str, std::vector &output) { size_t next = 0; bool even = 0; for (size_t pos = 0, len = str.length(); pos < len; ++pos) { @@ -372,11 +372,11 @@ void GetQuotedStrings(const std::string& str, std::vector &output) } } +// TODO: this is quite inefficient. std::string ReplaceAll(std::string_view input, std::string_view src, std::string_view dest) { size_t pos = 0; std::string result(input); - if (src == dest) return result; diff --git a/Common/StringUtils.h b/Common/StringUtils.h index da1ddf7468..2930d419e5 100644 --- a/Common/StringUtils.h +++ b/Common/StringUtils.h @@ -86,7 +86,7 @@ void SplitString(std::string_view str, const char delim, std::vector &output); -void GetQuotedStrings(const std::string& str, std::vector& output); +void GetQuotedStrings(std::string_view str, std::vector &output); std::string ReplaceAll(std::string_view input, std::string_view src, std::string_view dest); From 9d0c127961d06321d40519225d1a9919673ae634 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Sun, 5 May 2024 12:04:58 +0200 Subject: [PATCH 3/6] IniFile: Switch keys to string_view (avoids recomputing length every time) --- Common/Data/Format/IniFile.cpp | 57 ++++++++++++++-------------------- Common/Data/Format/IniFile.h | 56 ++++++++++++++++----------------- 2 files changed, 52 insertions(+), 61 deletions(-) diff --git a/Common/Data/Format/IniFile.cpp b/Common/Data/Format/IniFile.cpp index 43e3ca8822..829f09097b 100644 --- a/Common/Data/Format/IniFile.cpp +++ b/Common/Data/Format/IniFile.cpp @@ -198,7 +198,7 @@ bool Section::GetKeys(std::vector &keys) const { return true; } -ParsedIniLine *Section::GetLine(const char *key) { +ParsedIniLine *Section::GetLine(std::string_view key) { for (auto &line : lines_) { if (equalsNoCase(line.Key(), key)) return &line; @@ -206,7 +206,7 @@ ParsedIniLine *Section::GetLine(const char *key) { return nullptr; } -const ParsedIniLine *Section::GetLine(const char* key) const { +const ParsedIniLine *Section::GetLine(std::string_view key) const { for (auto &line : lines_) { if (equalsNoCase(line.Key(), key)) return &line; @@ -214,38 +214,38 @@ const ParsedIniLine *Section::GetLine(const char* key) const { return nullptr; } -void Section::Set(const char* key, uint32_t newValue) { +void Section::Set(std::string_view key, uint32_t newValue) { char temp[128]; snprintf(temp, sizeof(temp), "0x%08x", newValue); Set(key, (const char *)temp); } -void Section::Set(const char* key, uint64_t newValue) { +void Section::Set(std::string_view key, uint64_t newValue) { char temp[128]; snprintf(temp, sizeof(temp), "0x%016" PRIx64, newValue); Set(key, (const char *)temp); } -void Section::Set(const char* key, float newValue) { +void Section::Set(std::string_view key, float newValue) { _dbg_assert_(!my_isnanorinf(newValue)); char temp[128]; snprintf(temp, sizeof(temp), "%f", newValue); Set(key, (const char *)temp); } -void Section::Set(const char* key, double newValue) { +void Section::Set(std::string_view key, double newValue) { char temp[128]; snprintf(temp, sizeof(temp), "%f", newValue); Set(key, (const char *)temp); } -void Section::Set(const char* key, int newValue) { +void Section::Set(std::string_view key, int newValue) { char temp[128]; snprintf(temp, sizeof(temp), "%d", newValue); Set(key, (const char *)temp); } -void Section::Set(const char* key, const char* newValue) { +void Section::Set(std::string_view key, const char* newValue) { ParsedIniLine *line = GetLine(key); if (line) { line->SetValue(newValue); @@ -255,7 +255,7 @@ void Section::Set(const char* key, const char* newValue) { } } -void Section::Set(const char* key, const std::string& newValue, const std::string& defaultValue) +void Section::Set(std::string_view key, const std::string& newValue, const std::string& defaultValue) { if (newValue != defaultValue) Set(key, newValue); @@ -263,7 +263,7 @@ void Section::Set(const char* key, const std::string& newValue, const std::strin Delete(key); } -bool Section::Get(const char* key, std::string* value, const char* defaultValue) const { +bool Section::Get(std::string_view key, std::string* value, const char* defaultValue) const { const ParsedIniLine *line = GetLine(key); if (!line) { if (defaultValue) { @@ -276,7 +276,7 @@ bool Section::Get(const char* key, std::string* value, const char* defaultValue) return true; } -void Section::Set(const char* key, const float newValue, const float defaultValue) +void Section::Set(std::string_view key, const float newValue, const float defaultValue) { if (newValue != defaultValue) Set(key, newValue); @@ -284,7 +284,7 @@ void Section::Set(const char* key, const float newValue, const float defaultValu Delete(key); } -void Section::Set(const char* key, int newValue, int defaultValue) +void Section::Set(std::string_view key, int newValue, int defaultValue) { if (newValue != defaultValue) Set(key, newValue); @@ -292,7 +292,7 @@ void Section::Set(const char* key, int newValue, int defaultValue) Delete(key); } -void Section::Set(const char* key, bool newValue, bool defaultValue) +void Section::Set(std::string_view key, bool newValue, bool defaultValue) { if (newValue != defaultValue) Set(key, newValue); @@ -300,7 +300,7 @@ void Section::Set(const char* key, bool newValue, bool defaultValue) Delete(key); } -void Section::Set(const char* key, const std::vector& newValues) +void Section::Set(std::string_view key, const std::vector& newValues) { std::string temp; // Join the strings with , @@ -319,12 +319,10 @@ void Section::AddComment(const std::string &comment) { lines_.emplace_back(ParsedIniLine::CommentOnly("# " + comment)); } -bool Section::Get(const char* key, std::vector& values) const -{ +bool Section::Get(std::string_view key, std::vector& values) const { std::string temp; bool retval = Get(key, &temp, 0); - if (!retval || temp.empty()) - { + if (!retval || temp.empty()) { return false; } // ignore starting , if any @@ -333,7 +331,6 @@ bool Section::Get(const char* key, std::vector& values) const // split by , while (subStart != std::string::npos) { - // Find next , subEnd = temp.find_first_of(',', subStart); if (subStart != subEnd) @@ -347,8 +344,7 @@ bool Section::Get(const char* key, std::vector& values) const return true; } -bool Section::Get(const char* key, int* value, int defaultValue) const -{ +bool Section::Get(std::string_view key, int* value, int defaultValue) const { std::string temp; bool retval = Get(key, &temp, 0); if (retval && TryParse(temp, value)) @@ -357,8 +353,7 @@ bool Section::Get(const char* key, int* value, int defaultValue) const return false; } -bool Section::Get(const char* key, uint32_t* value, uint32_t defaultValue) const -{ +bool Section::Get(std::string_view key, uint32_t* value, uint32_t defaultValue) const { std::string temp; bool retval = Get(key, &temp, 0); if (retval && TryParse(temp, value)) @@ -367,8 +362,7 @@ bool Section::Get(const char* key, uint32_t* value, uint32_t defaultValue) const return false; } -bool Section::Get(const char* key, uint64_t* value, uint64_t defaultValue) const -{ +bool Section::Get(std::string_view key, uint64_t* value, uint64_t defaultValue) const { std::string temp; bool retval = Get(key, &temp, 0); if (retval && TryParse(temp, value)) @@ -377,8 +371,7 @@ bool Section::Get(const char* key, uint64_t* value, uint64_t defaultValue) const return false; } -bool Section::Get(const char* key, bool* value, bool defaultValue) const -{ +bool Section::Get(std::string_view key, bool* value, bool defaultValue) const { std::string temp; bool retval = Get(key, &temp, 0); if (retval && TryParse(temp, value)) @@ -387,8 +380,7 @@ bool Section::Get(const char* key, bool* value, bool defaultValue) const return false; } -bool Section::Get(const char* key, float* value, float defaultValue) const -{ +bool Section::Get(std::string_view key, float* value, float defaultValue) const { std::string temp; bool retval = Get(key, &temp, 0); if (retval && TryParse(temp, value)) @@ -397,8 +389,7 @@ bool Section::Get(const char* key, float* value, float defaultValue) const return false; } -bool Section::Get(const char* key, double* value, double defaultValue) const -{ +bool Section::Get(std::string_view key, double* value, double defaultValue) const { std::string temp; bool retval = Get(key, &temp, 0); if (retval && TryParse(temp, value)) @@ -407,7 +398,7 @@ bool Section::Get(const char* key, double* value, double defaultValue) const return false; } -bool Section::Exists(const char *key) const { +bool Section::Exists(std::string_view key) const { for (auto &line : lines_) { if (equalsNoCase(key, line.Key())) return true; @@ -425,7 +416,7 @@ std::map Section::ToMap() const { return outMap; } -bool Section::Delete(const char *key) { +bool Section::Delete(std::string_view key) { ParsedIniLine *line = GetLine(key); for (auto liter = lines_.begin(); liter != lines_.end(); ++liter) { if (line == &*liter) { diff --git a/Common/Data/Format/IniFile.h b/Common/Data/Format/IniFile.h index d5161787fe..8e944f8d1e 100644 --- a/Common/Data/Format/IniFile.h +++ b/Common/Data/Format/IniFile.h @@ -54,56 +54,56 @@ class Section { public: Section() {} - Section(const std::string& name) : name_(name) {} + Section(std::string_view name) : name_(name) {} - bool Exists(const char *key) const; - bool Delete(const char *key); + bool Exists(std::string_view key) const; + bool Delete(std::string_view key); void Clear(); std::map ToMap() const; - ParsedIniLine *GetLine(const char *key); - const ParsedIniLine *GetLine(const char *key) const; + ParsedIniLine *GetLine(std::string_view key); + const ParsedIniLine *GetLine(std::string_view key) const; - void Set(const char* key, const char* newValue); - void Set(const char* key, const std::string& newValue, const std::string& defaultValue); + void Set(std::string_view key, const char* newValue); + void Set(std::string_view key, const std::string& newValue, const std::string& defaultValue); - void Set(const std::string &key, const std::string &value) { - Set(key.c_str(), value.c_str()); + void Set(std::string_view key, const std::string &value) { + Set(key, value.c_str()); } - bool Get(const char* key, std::string* value, const char* defaultValue) const; + bool Get(std::string_view key, std::string* value, const char* defaultValue) const; - void Set(const char* key, uint32_t newValue); - void Set(const char* key, uint64_t newValue); - void Set(const char* key, float newValue); - void Set(const char* key, const float newValue, const float defaultValue); - void Set(const char* key, double newValue); + void Set(std::string_view key, uint32_t newValue); + void Set(std::string_view key, uint64_t newValue); + void Set(std::string_view key, float newValue); + void Set(std::string_view key, const float newValue, const float defaultValue); + void Set(std::string_view key, double newValue); - void Set(const char* key, int newValue, int defaultValue); - void Set(const char* key, int newValue); + void Set(std::string_view key, int newValue, int defaultValue); + void Set(std::string_view key, int newValue); - void Set(const char* key, bool newValue, bool defaultValue); - void Set(const char* key, bool newValue) { + void Set(std::string_view key, bool newValue, bool defaultValue); + void Set(std::string_view key, bool newValue) { Set(key, newValue ? "True" : "False"); } - void Set(const char* key, const std::vector& newValues); + void Set(std::string_view key, const std::vector& newValues); // Declare without a body to make it fail to compile. This is to prevent accidentally // setting a pointer as a bool. The failure is in the linker unfortunately, but that's better // than accidentally succeeding in a bad way. template - void Set(const char *key, T *ptr); + void Set(std::string_view key, T *ptr); void AddComment(const std::string &comment); - bool Get(const char* key, int* value, int defaultValue = 0) const; - bool Get(const char* key, uint32_t* value, uint32_t defaultValue = 0) const; - bool Get(const char* key, uint64_t* value, uint64_t defaultValue = 0) const; - bool Get(const char* key, bool* value, bool defaultValue = false) const; - bool Get(const char* key, float* value, float defaultValue = false) const; - bool Get(const char* key, double* value, double defaultValue = false) const; - bool Get(const char* key, std::vector& values) const; + bool Get(std::string_view key, int* value, int defaultValue = 0) const; + bool Get(std::string_view key, uint32_t* value, uint32_t defaultValue = 0) const; + bool Get(std::string_view key, uint64_t* value, uint64_t defaultValue = 0) const; + bool Get(std::string_view key, bool* value, bool defaultValue = false) const; + bool Get(std::string_view key, float* value, float defaultValue = false) const; + bool Get(std::string_view key, double* value, double defaultValue = false) const; + bool Get(std::string_view key, std::vector& values) const; // Return a list of all keys in this section bool GetKeys(std::vector &keys) const; From 85f8fd0bdbcfd249536c25aed69da52bd903b79a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Sun, 5 May 2024 12:08:41 +0200 Subject: [PATCH 4/6] Safety fix for GetCharPointer Uses IsValidNullTerminatedString from #19103 by Kethen --- Core/MemMap.h | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/Core/MemMap.h b/Core/MemMap.h index 56acfe823b..247d4d3478 100644 --- a/Core/MemMap.h +++ b/Core/MemMap.h @@ -270,16 +270,6 @@ inline bool IsKernelAndNotVolatileAddress(const u32 address) { bool IsScratchpadAddress(const u32 address); -// Used for auto-converted char * parameters, which can sometimes legitimately be null - -// so we don't want to get caught in GetPointer's crash reporting. -inline const char* GetCharPointer(const u32 address) { - if (address) { - return (const char *)GetPointer(address); - } else { - return nullptr; - } -} - inline const char *GetCharPointerUnchecked(const u32 address) { return (const char *)GetPointerUnchecked(address); } @@ -351,6 +341,16 @@ inline bool IsValidRange(const u32 address, const u32 size) { return ValidSize(address, size) == size; } +// Used for auto-converted char * parameters, which can sometimes legitimately be null - +// so we don't want to get caught in GetPointer's crash reporting. +inline const char *GetCharPointer(const u32 address) { + if (address && IsValidNullTerminatedString(address)) { + return GetCharPointerUnchecked(address); + } else { + return nullptr; + } +} + } // namespace Memory // Avoiding a global include for NotifyMemInfo. From c004bea5e463f6d8c97ee7a96da21fca80cfde75 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Sun, 5 May 2024 12:17:29 +0200 Subject: [PATCH 5/6] Use memchr in IsValidNullTerminatedString --- Core/MemMap.h | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/Core/MemMap.h b/Core/MemMap.h index 247d4d3478..b9f01fb506 100644 --- a/Core/MemMap.h +++ b/Core/MemMap.h @@ -270,10 +270,6 @@ inline bool IsKernelAndNotVolatileAddress(const u32 address) { bool IsScratchpadAddress(const u32 address); -inline const char *GetCharPointerUnchecked(const u32 address) { - return (const char *)GetPointerUnchecked(address); -} - inline void MemcpyUnchecked(void *to_data, const u32 from_address, const u32 len) { memcpy(to_data, GetPointerUnchecked(from_address), len); } @@ -314,6 +310,8 @@ inline u32 MaxSizeAtAddress(const u32 address){ } } +// NOTE: Unlike the similar IsValidRange/IsValidAddress functions, this one is linear cost vs the size of the string, +// for hopefully-obvious reasons. inline bool IsValidNullTerminatedString(const u32 address) { u32 max_size = MaxSizeAtAddress(address); if (max_size == 0) { @@ -321,10 +319,8 @@ inline bool IsValidNullTerminatedString(const u32 address) { } const char *c = GetCharPointerUnchecked(address); - for (u32 i = 0; i < max_size; i++) { - if (c[i] == '\0') { - return true; - } + if (memchr(c, '\0', max_size)) { + return true; } return false; } @@ -341,6 +337,10 @@ inline bool IsValidRange(const u32 address, const u32 size) { return ValidSize(address, size) == size; } +inline const char *GetCharPointerUnchecked(const u32 address) { + return (const char *)GetPointerUnchecked(address); +} + // Used for auto-converted char * parameters, which can sometimes legitimately be null - // so we don't want to get caught in GetPointer's crash reporting. inline const char *GetCharPointer(const u32 address) { From 0b91aa50eeb80fbb2999c542342a908736b28d65 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Sun, 5 May 2024 12:23:43 +0200 Subject: [PATCH 6/6] Improve memory safety, add bugfix to little-used sysclib functions --- Core/HLE/sceIo.cpp | 23 +++++++++++++++++------ Core/HLE/sceKernelInterrupt.cpp | 28 ++++++++++++++-------------- Core/HLE/sceKernelModule.cpp | 2 +- Core/MemMap.h | 13 +++++++------ 4 files changed, 39 insertions(+), 27 deletions(-) diff --git a/Core/HLE/sceIo.cpp b/Core/HLE/sceIo.cpp index d370f81438..974a224a26 100644 --- a/Core/HLE/sceIo.cpp +++ b/Core/HLE/sceIo.cpp @@ -838,6 +838,12 @@ static void IoStartAsyncThread(int id, FileNode *f) { static u32 sceIoAssign(u32 alias_addr, u32 physical_addr, u32 filesystem_addr, int mode, u32 arg_addr, int argSize) { + if (!Memory::IsValidNullTerminatedString(alias_addr) || + !Memory::IsValidNullTerminatedString(physical_addr) || + !Memory::IsValidNullTerminatedString(filesystem_addr)) { + return hleLogError(SCEIO, -1, "Bad parameters"); + } + std::string alias = Memory::GetCharPointer(alias_addr); std::string physical_dev = Memory::GetCharPointer(physical_addr); std::string filesystem_dev = Memory::GetCharPointer(filesystem_addr); @@ -2009,8 +2015,7 @@ static u32 sceIoDevctl(const char *name, int cmd, u32 argAddr, int argLen, u32 o { // Emulator special tricks! - enum - { + enum { EMULATOR_DEVCTL__GET_HAS_DISPLAY = 1, EMULATOR_DEVCTL__SEND_OUTPUT, EMULATOR_DEVCTL__IS_EMULATOR, @@ -2031,14 +2036,14 @@ static u32 sceIoDevctl(const char *name, int cmd, u32 argAddr, int argLen, u32 o Memory::Write_U32(PSP_CoreParameter().headLess ? 0 : 1, outPtr); return 0; case EMULATOR_DEVCTL__SEND_OUTPUT: - { - std::string data(Memory::GetCharPointer(argAddr), argLen); + if (Memory::IsValidRange(argAddr, argLen)) { + std::string data(Memory::GetCharPointerUnchecked(argAddr), argLen); if (!System_SendDebugOutput(data)) DEBUG_LOG(SCEIO, "%s", data.c_str()); if (PSP_CoreParameter().collectDebugOutput) *PSP_CoreParameter().collectDebugOutput += data; - return 0; } + return 0; case EMULATOR_DEVCTL__IS_EMULATOR: if (Memory::IsValidAddress(outPtr)) Memory::Write_U32(1, outPtr); @@ -2932,7 +2937,13 @@ static int IoAsyncFinish(int id) { case IoAsyncOp::OPEN: { // See notes on timing in sceIoOpen. - const std::string filename = Memory::GetCharPointer(params.open.filenameAddr); + if (!Memory::IsValidNullTerminatedString(params.open.filenameAddr)) { + // Bad + ERROR_LOG(SCEIO, "Bad pointer to filename %08x", params.open.filenameAddr); + us = 80; + break; + } + const std::string filename = Memory::GetCharPointerUnchecked(params.open.filenameAddr); IFileSystem *sys = pspFileSystem.GetSystemFromFilename(filename); if (sys) { if (f->asyncResult == (int)SCE_KERNEL_ERROR_ERRNO_FILE_NOT_FOUND) { diff --git a/Core/HLE/sceKernelInterrupt.cpp b/Core/HLE/sceKernelInterrupt.cpp index 85e2bf425c..33dcf2f303 100644 --- a/Core/HLE/sceKernelInterrupt.cpp +++ b/Core/HLE/sceKernelInterrupt.cpp @@ -697,7 +697,7 @@ static u32 sysclib_memcpy(u32 dst, u32 src, u32 size) { static u32 sysclib_strcat(u32 dst, u32 src) { ERROR_LOG(SCEKERNEL, "Untested sysclib_strcat(dest=%08x, src=%08x)", dst, src); - if (Memory::IsValidAddress(dst) && Memory::IsValidAddress(src)) { + if (Memory::IsValidNullTerminatedString(dst) && Memory::IsValidNullTerminatedString(src)) { strcat((char *)Memory::GetPointerWriteUnchecked(dst), (const char *)Memory::GetPointerUnchecked(src)); } return dst; @@ -705,7 +705,7 @@ static u32 sysclib_strcat(u32 dst, u32 src) { static int sysclib_strcmp(u32 dst, u32 src) { ERROR_LOG(SCEKERNEL, "Untested sysclib_strcmp(dest=%08x, src=%08x)", dst, src); - if (Memory::IsValidAddress(dst) && Memory::IsValidAddress(src)) { + if (Memory::IsValidNullTerminatedString(dst) && Memory::IsValidNullTerminatedString(src)) { return strcmp((const char *)Memory::GetPointerUnchecked(dst), (const char *)Memory::GetPointerUnchecked(src)); } else { // What to do? Crash, probably. @@ -715,7 +715,7 @@ static int sysclib_strcmp(u32 dst, u32 src) { static u32 sysclib_strcpy(u32 dst, u32 src) { ERROR_LOG(SCEKERNEL, "Untested sysclib_strcpy(dest=%08x, src=%08x)", dst, src); - if (Memory::IsValidAddress(dst) && Memory::IsValidAddress(src)) { + if (Memory::IsValidAddress(dst) && Memory::IsValidNullTerminatedString(src)) { strcpy((char *)Memory::GetPointerWriteUnchecked(dst), (const char *)Memory::GetPointerUnchecked(src)); } return dst; @@ -723,7 +723,7 @@ static u32 sysclib_strcpy(u32 dst, u32 src) { static u32 sysclib_strlen(u32 src) { ERROR_LOG(SCEKERNEL, "Untested sysclib_strlen(src=%08x)", src); - if (Memory::IsValidAddress(src)) { + if (Memory::IsValidNullTerminatedString(src)) { // TODO: This computes the length, could reuse it maybe. return (u32)strlen(Memory::GetCharPointerUnchecked(src)); } else { // What to do? Crash, probably. @@ -893,7 +893,7 @@ static int sysclib_sprintf(u32 dst, u32 fmt) { } static u32 sysclib_memset(u32 destAddr, int data, int size) { - ERROR_LOG(SCEKERNEL, "Untested sysclib_memset(dest=%08x, data=%d ,size=%d)", destAddr, data, size); + DEBUG_LOG(SCEKERNEL, "Untested sysclib_memset(dest=%08x, data=%d ,size=%d)", destAddr, data, size); if (Memory::IsValidRange(destAddr, size)) { memset(Memory::GetPointerWriteUnchecked(destAddr), data, size); } @@ -902,8 +902,8 @@ static u32 sysclib_memset(u32 destAddr, int data, int size) { } static int sysclib_strstr(u32 s1, u32 s2) { - ERROR_LOG(SCEKERNEL, "Untested sysclib_strstr(%08x, %08x)", s1, s2); - if (Memory::IsValidAddress(s1) && Memory::IsValidAddress(s2)) { + DEBUG_LOG(SCEKERNEL, "Untested sysclib_strstr(%08x, %08x)", s1, s2); + if (Memory::IsValidNullTerminatedString(s1) && Memory::IsValidNullTerminatedString(s2)) { std::string str1 = Memory::GetCharPointerUnchecked(s1); std::string str2 = Memory::GetCharPointerUnchecked(s2); size_t index = str1.find(str2); @@ -916,8 +916,8 @@ static int sysclib_strstr(u32 s1, u32 s2) { } static int sysclib_strncmp(u32 s1, u32 s2, u32 size) { - ERROR_LOG(SCEKERNEL, "Untested sysclib_strncmp(%08x, %08x, %08x)", s1, s2, size); - if (Memory::IsValidAddress(s1) && Memory::IsValidAddress(s2)) { + DEBUG_LOG(SCEKERNEL, "Untested sysclib_strncmp(%08x, %08x, %08x)", s1, s2, size); + if (Memory::IsValidRange(s1, size) && Memory::IsValidRange(s2, size)) { const char * str1 = Memory::GetCharPointerUnchecked(s1); const char * str2 = Memory::GetCharPointerUnchecked(s2); return strncmp(str1, str2, size); @@ -926,7 +926,7 @@ static int sysclib_strncmp(u32 s1, u32 s2, u32 size) { } static u32 sysclib_memmove(u32 dst, u32 src, u32 size) { - ERROR_LOG(SCEKERNEL, "Untested sysclib_memmove(%08x, %08x, %08x)", dst, src, size); + DEBUG_LOG(SCEKERNEL, "Untested sysclib_memmove(%08x, %08x, %08x)", dst, src, size); if (Memory::IsValidRange(dst, size) && Memory::IsValidRange(src, size)) { memmove(Memory::GetPointerWriteUnchecked(dst), Memory::GetPointerUnchecked(src), size); } @@ -937,7 +937,7 @@ static u32 sysclib_memmove(u32 dst, u32 src, u32 size) { } static u32 sysclib_strncpy(u32 dest, u32 src, u32 size) { - if (!Memory::IsValidAddress(dest) || Memory::IsValidAddress(src)) { + if (!Memory::IsValidAddress(dest) || !Memory::IsValidAddress(src)) { return hleLogError(SCEKERNEL, 0, "invalid address"); } @@ -962,7 +962,7 @@ static u32 sysclib_strncpy(u32 dest, u32 src, u32 size) { } static u32 sysclib_strtol(u32 strPtr, u32 endPtrPtr, int base) { - if (!Memory::IsValidAddress(strPtr)) { + if (!Memory::IsValidNullTerminatedString(strPtr)) { return hleLogError(SCEKERNEL, 0, "invalid address"); } const char* str = Memory::GetCharPointer(strPtr); @@ -974,7 +974,7 @@ static u32 sysclib_strtol(u32 strPtr, u32 endPtrPtr, int base) { } static u32 sysclib_strchr(u32 src, int c) { - if (!Memory::IsValidAddress(src)) { + if (!Memory::IsValidNullTerminatedString(src)) { return hleLogError(SCEKERNEL, 0, "invalid address"); } const std::string str = Memory::GetCharPointer(src); @@ -986,7 +986,7 @@ static u32 sysclib_strchr(u32 src, int c) { } static u32 sysclib_strrchr(u32 src, int c) { - if (!Memory::IsValidAddress(src)) { + if (!Memory::IsValidNullTerminatedString(src)) { return hleLogError(SCEKERNEL, 0, "invalid address"); } const std::string str = Memory::GetCharPointer(src); diff --git a/Core/HLE/sceKernelModule.cpp b/Core/HLE/sceKernelModule.cpp index bd61e25b75..409698cea8 100644 --- a/Core/HLE/sceKernelModule.cpp +++ b/Core/HLE/sceKernelModule.cpp @@ -1814,7 +1814,7 @@ bool __KernelLoadExec(const char *filename, u32 paramPtr, std::string *error_str } if (param.keyp != 0) { u32 keyAddr = param.keyp; - size_t keylen = strlen(Memory::GetCharPointer(keyAddr))+1; + size_t keylen = strlen(Memory::GetCharPointer(keyAddr)) + 1; param_key = new u8[keylen]; Memory::Memcpy(param_key, keyAddr, (u32)keylen, "KernelLoadParam"); } diff --git a/Core/MemMap.h b/Core/MemMap.h index b9f01fb506..c53a75f4a4 100644 --- a/Core/MemMap.h +++ b/Core/MemMap.h @@ -310,6 +310,10 @@ inline u32 MaxSizeAtAddress(const u32 address){ } } +inline const char *GetCharPointerUnchecked(const u32 address) { + return (const char *)GetPointerUnchecked(address); +} + // NOTE: Unlike the similar IsValidRange/IsValidAddress functions, this one is linear cost vs the size of the string, // for hopefully-obvious reasons. inline bool IsValidNullTerminatedString(const u32 address) { @@ -337,14 +341,11 @@ inline bool IsValidRange(const u32 address, const u32 size) { return ValidSize(address, size) == size; } -inline const char *GetCharPointerUnchecked(const u32 address) { - return (const char *)GetPointerUnchecked(address); -} - // Used for auto-converted char * parameters, which can sometimes legitimately be null - -// so we don't want to get caught in GetPointer's crash reporting. +// so we don't want to get caught in GetPointer's crash reporting +// TODO: This should use IsValidNullTerminatedString, but may be expensive since this is used so much - needs evaluation. inline const char *GetCharPointer(const u32 address) { - if (address && IsValidNullTerminatedString(address)) { + if (address && IsValidAddress(address)) { return GetCharPointerUnchecked(address); } else { return nullptr;