From 2b558eb545e6998b92ec1c60a4134dfd132a4948 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Sun, 23 Mar 2025 07:10:04 +0100 Subject: [PATCH] Fix regression in Tomb Raider Legends Need to make sure a buffer size check happens before the buffer is accessed - since the game calls sceAtracSetDataAndGetID with both buffer and bufferSize as 0, and it expects to receive TOO_SMALL and not a memory exception. Thanks to @sum2012 for quick investigationh help. --- Core/HLE/sceAtrac.cpp | 22 ++++++++++++---------- Core/MemMap.h | 5 +++++ Core/Util/AtracTrack.cpp | 19 ++++++++++++++++++- 3 files changed, 35 insertions(+), 11 deletions(-) diff --git a/Core/HLE/sceAtrac.cpp b/Core/HLE/sceAtrac.cpp index d2ae75e39e..d231985edd 100644 --- a/Core/HLE/sceAtrac.cpp +++ b/Core/HLE/sceAtrac.cpp @@ -637,7 +637,7 @@ static u32 sceAtracSetHalfwayBuffer(int atracID, u32 buffer, u32 readSize, u32 b Track track; std::string error; - int ret = AnalyzeAtracTrack(Memory::GetPointer(buffer), readSize, &track, &error); + int ret = AnalyzeAtracTrack(Memory::GetPointerOrNull(buffer), readSize, &track, &error); if (ret < 0) { return hleLogError(Log::ME, ret, "%s", error.c_str()); } @@ -673,7 +673,7 @@ static u32 sceAtracSetData(int atracID, u32 buffer, u32 bufferSize) { Track track; std::string error; - int ret = AnalyzeAtracTrack(Memory::GetPointer(buffer), bufferSize, &track, &error); + int ret = AnalyzeAtracTrack(Memory::GetPointerOrNull(buffer), bufferSize, &track, &error); if (ret < 0) { return hleLogError(Log::ME, ret, "%s", error.c_str()); } @@ -701,7 +701,9 @@ static int sceAtracSetDataAndGetID(u32 buffer, int bufferSize) { Track track; std::string error; - int ret = AnalyzeAtracTrack(Memory::GetPointer(buffer), bufferSize, &track, &error); + // We let zero-pointer through as zeroes, so the small size check can kick in before it's accessed. + // This is needed in Tomb Raider Legend (#20145). + int ret = AnalyzeAtracTrack(Memory::GetPointerOrNull(buffer), bufferSize, &track, &error); if (ret < 0) { return hleLogError(Log::ME, ret, "%s", error.c_str()); } @@ -727,7 +729,7 @@ static int sceAtracSetHalfwayBufferAndGetID(u32 buffer, u32 readSize, u32 buffer Track track; std::string error; - int ret = AnalyzeAtracTrack(Memory::GetPointer(buffer), readSize, &track, &error); + int ret = AnalyzeAtracTrack(Memory::GetPointerOrNull(buffer), readSize, &track, &error); if (ret < 0) { return hleLogError(Log::ME, ret, "%s", error.c_str()); } @@ -847,7 +849,7 @@ static int sceAtracSetMOutHalfwayBuffer(int atracID, u32 buffer, u32 readSize, u Track track; std::string error; - int ret = AnalyzeAtracTrack(Memory::GetPointer(buffer), readSize, &track, &error); + int ret = AnalyzeAtracTrack(Memory::GetPointerOrNull(buffer), readSize, &track, &error); if (ret < 0) { return hleLogError(Log::ME, ret, "%s", error.c_str()); } @@ -872,7 +874,7 @@ static u32 sceAtracSetMOutData(int atracID, u32 buffer, u32 bufferSize) { Track track; std::string error; - int ret = AnalyzeAtracTrack(Memory::GetPointer(buffer), bufferSize, &track, &error); + int ret = AnalyzeAtracTrack(Memory::GetPointerOrNull(buffer), bufferSize, &track, &error); if (ret < 0) { return hleLogError(Log::ME, ret, "%s", error.c_str()); } @@ -891,7 +893,7 @@ static u32 sceAtracSetMOutData(int atracID, u32 buffer, u32 bufferSize) { static int sceAtracSetMOutDataAndGetID(u32 buffer, u32 bufferSize) { Track track; std::string error; - int ret = AnalyzeAtracTrack(Memory::GetPointer(buffer), bufferSize, &track, &error); + int ret = AnalyzeAtracTrack(Memory::GetPointerOrNull(buffer), bufferSize, &track, &error); if (ret < 0) { return hleLogError(Log::ME, ret, "%s", error.c_str()); } @@ -920,7 +922,7 @@ static int sceAtracSetMOutHalfwayBufferAndGetID(u32 buffer, u32 readSize, u32 bu Track track; std::string error; - int ret = AnalyzeAtracTrack(Memory::GetPointer(buffer), readSize, &track, &error); + int ret = AnalyzeAtracTrack(Memory::GetPointerOrNull(buffer), readSize, &track, &error); if (ret < 0) { return hleLogError(Log::ME, ret, "%s", error.c_str()); } @@ -945,7 +947,7 @@ static int sceAtracSetMOutHalfwayBufferAndGetID(u32 buffer, u32 readSize, u32 bu static int sceAtracSetAA3DataAndGetID(u32 buffer, u32 bufferSize, u32 fileSize, u32 metadataSizeAddr) { Track track; std::string error; - int ret = AnalyzeAtracTrack(Memory::GetPointer(buffer), bufferSize, &track, &error); + int ret = AnalyzeAtracTrack(Memory::GetPointerOrNull(buffer), bufferSize, &track, &error); if (ret < 0) { return hleLogError(Log::ME, ret, "%s", error.c_str()); } @@ -971,7 +973,7 @@ static int sceAtracSetAA3HalfwayBufferAndGetID(u32 buffer, u32 readSize, u32 buf Track track; std::string error; - int ret = AnalyzeAtracTrack(Memory::GetPointer(buffer), readSize, &track, &error); + int ret = AnalyzeAtracTrack(Memory::GetPointerOrNull(buffer), readSize, &track, &error); if (ret < 0) { return hleLogError(Log::ME, ret, "%s", error.c_str()); } diff --git a/Core/MemMap.h b/Core/MemMap.h index 8a91637812..32dadf117d 100644 --- a/Core/MemMap.h +++ b/Core/MemMap.h @@ -396,6 +396,11 @@ inline u32 GetAddressFromHostPointer(const void* host_ptr) { return address; } +// Like GetPointer, but bad values don't result in a memory exception, instead nullptr is returned. +inline const u8* GetPointerOrNull(const u32 address) { + return IsValidAddress(address) ? GetPointerUnchecked(address) : nullptr; +} + } // namespace Memory // Avoiding a global include for NotifyMemInfo. diff --git a/Core/Util/AtracTrack.cpp b/Core/Util/AtracTrack.cpp index e40c6d5555..2c23cfaf4f 100644 --- a/Core/Util/AtracTrack.cpp +++ b/Core/Util/AtracTrack.cpp @@ -25,6 +25,16 @@ static u32 Read32(const u8 *buffer, int offset) { int AnalyzeAtracTrack(const u8 *buffer, u32 size, Track *track, std::string *error) { // 72 is about the size of the minimum required data to even be valid. + if (size < 72) { + return SCE_ERROR_ATRAC_SIZE_TOO_SMALL; + } + + // If the pointer is bad, let's try to survive, although I'm pretty sure that on a real PSP, + // we crash here. + if (!buffer) { + _dbg_assert_(false); + return SCE_KERNEL_ERROR_INVALID_POINTER; + } // TODO: Validate stuff more. if (Read32(buffer, 0) != RIFF_CHUNK_MAGIC) { @@ -238,10 +248,17 @@ int AnalyzeAtracTrack(const u8 *buffer, u32 size, Track *track, std::string *err } int AnalyzeAA3Track(const u8 *buffer, u32 size, u32 fileSize, Track *track, std::string *error) { + // TODO: Make sure this validation is correct, more testing. if (size < 10) { return SCE_ERROR_ATRAC_AA3_SIZE_TOO_SMALL; } - // TODO: Make sure this validation is correct, more testing. + + // If the pointer is bad, let's try to survive, although I'm pretty sure that on a real PSP, + // we crash here. + if (!buffer) { + _dbg_assert_(false); + return SCE_KERNEL_ERROR_INVALID_POINTER; + } if (buffer[0] != 'e' || buffer[1] != 'a' || buffer[2] != '3') { return SCE_ERROR_ATRAC_AA3_INVALID_DATA;