From 21686a6a934727a73e928b29fb64c5478d9ab119 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Thu, 7 Oct 2021 21:07:35 +0200 Subject: [PATCH 1/6] Android: Catch some exceptions --- .../src/org/ppsspp/ppsspp/NativeActivity.java | 51 ++++++++++++------- 1 file changed, 33 insertions(+), 18 deletions(-) diff --git a/android/src/org/ppsspp/ppsspp/NativeActivity.java b/android/src/org/ppsspp/ppsspp/NativeActivity.java index b62af3fbc305..fe7cba3de8fb 100644 --- a/android/src/org/ppsspp/ppsspp/NativeActivity.java +++ b/android/src/org/ppsspp/ppsspp/NativeActivity.java @@ -1118,30 +1118,39 @@ protected void onActivityResult(int requestCode, int resultCode, Intent data) { return; } if (requestCode == RESULT_LOAD_IMAGE) { - Uri selectedImage = data.getData(); - if (selectedImage != null) { - if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) { - NativeApp.sendMessage("bgImage_updated", selectedImage.toString()); - } else { - String[] filePathColumn = {MediaStore.Images.Media.DATA}; - Cursor cursor = getContentResolver().query(selectedImage, filePathColumn, null, null, null); - if (cursor != null) { - cursor.moveToFirst(); - int columnIndex = cursor.getColumnIndex(filePathColumn[0]); - String picturePath = cursor.getString(columnIndex); - cursor.close(); - - NativeApp.sendMessage("bgImage_updated", picturePath); + try { + Uri selectedImage = data.getData(); + if (selectedImage != null) { + if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) { + NativeApp.sendMessage("bgImage_updated", selectedImage.toString()); + } else { + String[] filePathColumn = {MediaStore.Images.Media.DATA}; + Cursor cursor = getContentResolver().query(selectedImage, filePathColumn, null, null, null); + if (cursor != null) { + cursor.moveToFirst(); + int columnIndex = cursor.getColumnIndex(filePathColumn[0]); + String picturePath = cursor.getString(columnIndex); + cursor.close(); + NativeApp.sendMessage("bgImage_updated", picturePath); + } } } + } catch (Exception e) { + Log.w(TAG, "Exception receiving image: " + e.toString()); } } else if (requestCode == RESULT_OPEN_DOCUMENT) { Uri selectedFile = data.getData(); if (selectedFile != null) { - // Grab permanent permission so we can show it in recents list etc. - if (Build.VERSION.SDK_INT >= 19) { - getContentResolver().takePersistableUriPermission(selectedFile, Intent.FLAG_GRANT_READ_URI_PERMISSION); + try { + // Grab permanent permission so we can show it in recents list etc. + if (Build.VERSION.SDK_INT >= 19) { + getContentResolver().takePersistableUriPermission(selectedFile, Intent.FLAG_GRANT_READ_URI_PERMISSION); + } + } catch (Exception e) { + Log.w(TAG, "Exception getting permissions for document: " + e.toString()); } + // Even if we got an exception getting permissions, try to pass along the file. Maybe this version of Android + // doesn't need it. Log.i(TAG, "Browse file finished:" + selectedFile.toString()); NativeApp.sendMessage("browse_fileSelect", selectedFile.toString()); } @@ -1150,7 +1159,13 @@ protected void onActivityResult(int requestCode, int resultCode, Intent data) { if (selectedDirectoryUri != null) { String path = selectedDirectoryUri.toString(); Log.i(TAG, "Browse folder finished: " + path); - getContentResolver().takePersistableUriPermission(selectedDirectoryUri, Intent.FLAG_GRANT_READ_URI_PERMISSION | Intent.FLAG_GRANT_WRITE_URI_PERMISSION); + try { + getContentResolver().takePersistableUriPermission(selectedDirectoryUri, Intent.FLAG_GRANT_READ_URI_PERMISSION | Intent.FLAG_GRANT_WRITE_URI_PERMISSION); + } catch (Exception e) { + Log.w(TAG, "Exception getting permissions for document: " + e.toString()); + } + // Even if we got an exception getting permissions, try to pass along the file. Maybe this version of Android + // doesn't need it. If we can't access it, we'll fail in some other way later. DocumentFile documentFile = DocumentFile.fromTreeUri(this, selectedDirectoryUri); Log.i(TAG, "Document name: " + documentFile.getUri()); /* From 9c017e03f9a9cecb1168a93468ca22dee30f3359 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Thu, 7 Oct 2021 21:08:12 +0200 Subject: [PATCH 2/6] Add some basic sanity checks to ParamSFO reader (could add more) --- Core/ELF/ParamSFO.cpp | 9 ++++++--- Core/ELF/ParamSFO.h | 6 +++++- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/Core/ELF/ParamSFO.cpp b/Core/ELF/ParamSFO.cpp index af5a0ce8123f..61b55281bae3 100644 --- a/Core/ELF/ParamSFO.cpp +++ b/Core/ELF/ParamSFO.cpp @@ -103,19 +103,22 @@ bool ParamSFOData::ReadSFO(const u8 *paramsfo, size_t size) { const IndexTable *indexTables = (const IndexTable *)(paramsfo + sizeof(Header)); + if (header->key_table_start > size || header->data_table_start > size) { + return false; + } + const u8 *key_start = paramsfo + header->key_table_start; const u8 *data_start = paramsfo + header->data_table_start; for (u32 i = 0; i < header->index_table_entries; i++) { const char *key = (const char *)(key_start + indexTables[i].key_table_offset); - switch (indexTables[i].param_fmt) { case 0x0404: { // Unsigned int const u32_le *data = (const u32_le *)(data_start + indexTables[i].data_table_offset); - SetValue(key,*data,indexTables[i].param_max_len); + SetValue(key, *data, indexTables[i].param_max_len); VERBOSE_LOG(LOADER, "%s %08x", key, *data); } break; @@ -132,7 +135,7 @@ bool ParamSFOData::ReadSFO(const u8 *paramsfo, size_t size) { { const char *utfdata = (const char *)(data_start + indexTables[i].data_table_offset); VERBOSE_LOG(LOADER, "%s %s", key, utfdata); - SetValue(key,std::string(utfdata /*, indexTables[i].param_len*/), indexTables[i].param_max_len); + SetValue(key, std::string(utfdata /*, indexTables[i].param_len*/), indexTables[i].param_max_len); } break; } diff --git a/Core/ELF/ParamSFO.h b/Core/ELF/ParamSFO.h index b0fee3cece95..6548638ef60d 100644 --- a/Core/ELF/ParamSFO.h +++ b/Core/ELF/ParamSFO.h @@ -56,7 +56,11 @@ class ParamSFOData bool WriteSFO(u8 **paramsfo, size_t *size); bool ReadSFO(const std::vector ¶msfo) { - return ReadSFO(¶msfo[0], paramsfo.size()); + if (!paramsfo.empty()) { + return ReadSFO(¶msfo[0], paramsfo.size()); + } else { + return false; + } } int GetDataOffset(const u8 *paramsfo, std::string dataName); From 36ada6308df2bef1f1bb96f5b9b11f5ae8f9e961 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Thu, 7 Oct 2021 21:08:46 +0200 Subject: [PATCH 3/6] Sanity check string lengths in save state code --- Common/Serialize/Serializer.cpp | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/Common/Serialize/Serializer.cpp b/Common/Serialize/Serializer.cpp index 6000328a7509..6b36fb21673e 100644 --- a/Common/Serialize/Serializer.cpp +++ b/Common/Serialize/Serializer.cpp @@ -106,10 +106,19 @@ void PointerWrap::DoVoid(void *data, int size) { (*ptr) += size; } +// Not exactly sane but might catch some corrupt files. +const int MAX_SANE_STRING_LENGTH = 1024 * 1024; + void Do(PointerWrap &p, std::string &x) { int stringLen = (int)x.length() + 1; Do(p, stringLen); + if (stringLen < 0 || stringLen > MAX_SANE_STRING_LENGTH) { + WARN_LOG(SAVESTATE, "Savestate failure: bad stringLen %d", stringLen); + p.SetError(PointerWrap::ERROR_FAILURE); + return; + } + switch (p.mode) { case PointerWrap::MODE_READ: x = (char*)*p.ptr; break; case PointerWrap::MODE_WRITE: memcpy(*p.ptr, x.c_str(), stringLen); break; @@ -123,6 +132,12 @@ void Do(PointerWrap &p, std::wstring &x) { int stringLen = sizeof(wchar_t) * ((int)x.length() + 1); Do(p, stringLen); + if (stringLen < 0 || stringLen > MAX_SANE_STRING_LENGTH) { + WARN_LOG(SAVESTATE, "Savestate failure: bad stringLen %d", stringLen); + p.SetError(PointerWrap::ERROR_FAILURE); + return; + } + auto read = [&]() { std::wstring r; // In case unaligned, use memcpy. @@ -144,6 +159,12 @@ void Do(PointerWrap &p, std::u16string &x) { int stringLen = sizeof(char16_t) * ((int)x.length() + 1); Do(p, stringLen); + if (stringLen < 0 || stringLen > MAX_SANE_STRING_LENGTH) { + WARN_LOG(SAVESTATE, "Savestate failure: bad stringLen %d", stringLen); + p.SetError(PointerWrap::ERROR_FAILURE); + return; + } + auto read = [&]() { std::u16string r; // In case unaligned, use memcpy. From 2257febc82996bbf844c3ad98d632ce979f5a805 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Thu, 7 Oct 2021 23:51:26 +0200 Subject: [PATCH 4/6] Avoid throwing exceptions on bad post shader float parameters --- Core/Config.cpp | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/Core/Config.cpp b/Core/Config.cpp index d8d9e11a44ed..305098f47cdd 100644 --- a/Core/Config.cpp +++ b/Core/Config.cpp @@ -1794,8 +1794,13 @@ bool Config::loadGameConfig(const std::string &pGameId, const std::string &title auto postShaderSetting = iniFile.GetOrCreateSection("PostShaderSetting")->ToMap(); mPostShaderSetting.clear(); - for (auto it : postShaderSetting) { - mPostShaderSetting[it.first] = std::stof(it.second); + for (const auto &it : postShaderSetting) { + float value = 0.0f; + if (sscanf(it.second.c_str(), "%f", &value)) { + mPostShaderSetting[it.first] = value; + } else { + WARN_LOG(LOADER, "Invalid float value string for param %s: '%s'", it.first.c_str(), it.second.c_str()); + } } auto postShaderChain = iniFile.GetOrCreateSection("PostShaderList")->ToMap(); From b069a239aaa75bbc5a939a6f06b516090e09963e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Thu, 7 Oct 2021 23:59:21 +0200 Subject: [PATCH 5/6] Add sanity check to DirectoryFileHandle::Read --- Core/FileSystems/DirectoryFileSystem.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Core/FileSystems/DirectoryFileSystem.cpp b/Core/FileSystems/DirectoryFileSystem.cpp index 9a3bb74cfb5c..9769563de8d1 100644 --- a/Core/FileSystems/DirectoryFileSystem.cpp +++ b/Core/FileSystems/DirectoryFileSystem.cpp @@ -311,11 +311,13 @@ size_t DirectoryFileHandle::Read(u8* pointer, s64 size) size = needsTrunc_ - off; } } + if (size > 0) { #ifdef _WIN32 - ::ReadFile(hFile, (LPVOID)pointer, (DWORD)size, (LPDWORD)&bytesRead, 0); + ::ReadFile(hFile, (LPVOID)pointer, (DWORD)size, (LPDWORD)&bytesRead, 0); #else - bytesRead = read(hFile, pointer, size); + bytesRead = read(hFile, pointer, size); #endif + } return replay_ ? ReplayApplyDiskRead(pointer, (uint32_t)bytesRead, (uint32_t)size, inGameDir_, CoreTiming::GetGlobalTimeUs()) : bytesRead; } From 7ec60f28b3f6e667ce4be9336b092a5255284c1b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Fri, 8 Oct 2021 00:11:44 +0200 Subject: [PATCH 6/6] Paper over a race condition slightly better. Really need to redo sync here. --- Core/Util/GameManager.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Core/Util/GameManager.cpp b/Core/Util/GameManager.cpp index 66b35235e542..67339624864c 100644 --- a/Core/Util/GameManager.cpp +++ b/Core/Util/GameManager.cpp @@ -597,12 +597,12 @@ bool GameManager::InstallMemstickGame(struct zip *z, const Path &zipfile, const zip_close(z); z = nullptr; installProgress_ = 1.0f; - installInProgress_ = false; installError_ = ""; if (deleteAfter) { File::Delete(zipfile); } InstallDone(); + installInProgress_ = false; return true; bail: @@ -647,9 +647,9 @@ bool GameManager::InstallZippedISO(struct zip *z, int isoFileIndex, const Path & z = 0; installProgress_ = 1.0f; - installInProgress_ = false; installError_ = ""; InstallDone(); + installInProgress_ = false; return true; } @@ -670,9 +670,9 @@ bool GameManager::InstallRawISO(const Path &file, const std::string &originalNam } } installProgress_ = 1.0f; - installInProgress_ = false; installError_ = ""; InstallDone(); + installInProgress_ = false; return true; }