From 0a687b9435a8f0171de187a8e89e4a1d4dcbc3c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Fri, 25 Sep 2026 20:27:29 -0600 Subject: [PATCH] Savestate: Bounds-check sizes from the file, and plug leaks on load Reject sizes past the end of the state before allocating (FPL, PGF, achievements, SAS grain, savedata list, the memory fast path), fail instead of desyncing on a SAS voice count mismatch, and free what old states' paths and shrinking pointer containers dropped. Co-Authored-By: Claude Opus 5.5 (1M context) --- Common/Serialize/SerializeDeque.h | 9 ++++++++- Common/Serialize/SerializeFuncs.h | 7 +++++++ Common/Serialize/SerializeList.h | 7 +++++++ Core/Dialog/SavedataParam.cpp | 9 +++++++++ Core/Font/PGF.cpp | 5 +++++ Core/HLE/sceFont.cpp | 3 ++- Core/HLE/sceHeap.cpp | 5 +++++ Core/HLE/sceKernelMemory.cpp | 7 ++++++- Core/HLE/sceMp3.cpp | 6 ++++++ Core/HW/SasAudio.cpp | 6 ++++++ Core/MemMap.cpp | 4 ++++ Core/RetroAchievements.cpp | 3 +++ 12 files changed, 68 insertions(+), 3 deletions(-) diff --git a/Common/Serialize/SerializeDeque.h b/Common/Serialize/SerializeDeque.h index 3c9a817884..6c0aaad8d8 100644 --- a/Common/Serialize/SerializeDeque.h +++ b/Common/Serialize/SerializeDeque.h @@ -27,7 +27,7 @@ void DoDeque(PointerWrap &p, std::deque &x, T &default_val) { Do(p, deq_size); // Guard against an attacker-controlled size driving a huge resize, same as DoVector. if (p.mode == PointerWrap::MODE_READ || p.mode == PointerWrap::MODE_VERIFY) { - if (deq_size > p.Remaining() / sizeof(T)) { + if (deq_size > p.Remaining() / SerializeMinElemSize()) { p.SetError(PointerWrap::ERROR_FAILURE); return; } @@ -40,6 +40,13 @@ void DoDeque(PointerWrap &p, std::deque &x, T &default_val) { template void Do(PointerWrap &p, std::deque &x) { + if (p.mode == PointerWrap::MODE_READ) { + // The elements are owned (DoClass replaces them), and a shorter deque would drop the rest. + for (T *elem : x) { + delete elem; + } + x.clear(); + } T *dv = nullptr; DoDeque(p, x, dv); } diff --git a/Common/Serialize/SerializeFuncs.h b/Common/Serialize/SerializeFuncs.h index 49fcded674..c1c2994318 100644 --- a/Common/Serialize/SerializeFuncs.h +++ b/Common/Serialize/SerializeFuncs.h @@ -126,6 +126,13 @@ void DoVector(PointerWrap &p, std::vector &x, T &default_val) { template void Do(PointerWrap &p, std::vector &x) { + if (p.mode == PointerWrap::MODE_READ) { + // The elements are owned (DoClass replaces them), and a shorter vector would drop the rest. + for (T *elem : x) { + delete elem; + } + x.clear(); + } T *dv = nullptr; DoVector(p, x, dv); } diff --git a/Common/Serialize/SerializeList.h b/Common/Serialize/SerializeList.h index f768ddabb6..3a37aaf891 100644 --- a/Common/Serialize/SerializeList.h +++ b/Common/Serialize/SerializeList.h @@ -40,6 +40,13 @@ void DoList(PointerWrap &p, std::list &x, T &default_val) { template void Do(PointerWrap &p, std::list &x) { + if (p.mode == PointerWrap::MODE_READ) { + // The elements are owned (DoClass replaces them), and a shorter list would drop the rest. + for (T *elem : x) { + delete elem; + } + x.clear(); + } T *dv = nullptr; Do(p, x, dv); } diff --git a/Core/Dialog/SavedataParam.cpp b/Core/Dialog/SavedataParam.cpp index ef0bec2bd9..d5ded0b5a5 100644 --- a/Core/Dialog/SavedataParam.cpp +++ b/Core/Dialog/SavedataParam.cpp @@ -1990,6 +1990,15 @@ void SavedataParam::DoState(PointerWrap &p) { Do(p, saveNameListDataCount); if (p.mode == p.MODE_READ) { delete [] saveDataList; + saveDataList = nullptr; + if (saveDataListCount < 0 || !p.CheckRead(saveDataListCount)) { + saveDataListCount = 0; + saveNameListDataCount = 0; + p.SetError(p.ERROR_FAILURE); + return; + } + // Clear() leaves the name count behind, but it's only used to walk the list. + saveNameListDataCount = std::clamp(saveNameListDataCount, 0, saveDataListCount); if (saveDataListCount != 0) { saveDataList = new SaveFileInfo[saveDataListCount]; DoArray(p, saveDataList, saveDataListCount); diff --git a/Core/Font/PGF.cpp b/Core/Font/PGF.cpp index 4c31ded955..18a6d9b07c 100644 --- a/Core/Font/PGF.cpp +++ b/Core/Font/PGF.cpp @@ -136,6 +136,11 @@ void PGF::DoState(PointerWrap &p) { fontDataSize = (size_t)fontDataSizeTemp; if (p.mode == p.MODE_READ) { delete [] fontData; + fontData = nullptr; + if (!p.CheckRead(fontDataSize)) { + fontDataSize = 0; + return; + } if (fontDataSize) { fontData = new u8[fontDataSize]; DoArray(p, fontData, (int)fontDataSize); diff --git a/Core/HLE/sceFont.cpp b/Core/HLE/sceFont.cpp index a2151d8b94..011dbfcbfd 100644 --- a/Core/HLE/sceFont.cpp +++ b/Core/HLE/sceFont.cpp @@ -360,7 +360,8 @@ public: if (s >= 3) { Do(p, mode_); } else { - mode_ = FONT_OPEN_INTERNAL_FULL; + // Only the destructor looks at this: a font loaded above is ours to delete. + mode_ = internalFont == -1 ? FONT_OPEN_USERBUFFER : FONT_OPEN_INTERNAL_FULL; } } diff --git a/Core/HLE/sceHeap.cpp b/Core/HLE/sceHeap.cpp index 9a77099316..ae91ec8632 100644 --- a/Core/HLE/sceHeap.cpp +++ b/Core/HLE/sceHeap.cpp @@ -62,6 +62,11 @@ void __HeapDoState(PointerWrap &p) { if (s >= 2) { Do(p, heapList); + } else if (p.mode == p.MODE_READ) { + for (auto &[_, heap] : heapList) { + delete heap; + } + heapList.clear(); } } diff --git a/Core/HLE/sceKernelMemory.cpp b/Core/HLE/sceKernelMemory.cpp index e8008425e8..57483e8a87 100644 --- a/Core/HLE/sceKernelMemory.cpp +++ b/Core/HLE/sceKernelMemory.cpp @@ -105,8 +105,13 @@ void FPL::DoState(PointerWrap &p) { return; Do(p, nf); - if (p.mode == p.MODE_READ) + if (p.mode == p.MODE_READ) { + if (nf.numBlocks < 0 || !p.CheckRead(nf.numBlocks)) { + p.SetError(p.ERROR_FAILURE); + return; + } blocks = new bool[nf.numBlocks]; + } DoArray(p, blocks, nf.numBlocks); Do(p, address); Do(p, alignedSize); diff --git a/Core/HLE/sceMp3.cpp b/Core/HLE/sceMp3.cpp index fff5c2f5d4..9e05bdcede 100644 --- a/Core/HLE/sceMp3.cpp +++ b/Core/HLE/sceMp3.cpp @@ -164,6 +164,11 @@ void __Mp3DoState(PointerWrap &p) { if (s >= 2) { Do(p, g_mp3Map); } else { + for (auto &[_, mp3] : g_mp3Map) { + delete mp3; + } + g_mp3Map.clear(); + std::map mp3Map_old; Do(p, mp3Map_old); // read old map for (auto it = mp3Map_old.begin(), end = mp3Map_old.end(); it != end; ++it) { @@ -188,6 +193,7 @@ void __Mp3DoState(PointerWrap &p) { mp3->decoder = CreateAudioDecoder(PSP_CODEC_MP3); g_mp3Map[id] = mp3; + delete mp3_old; } } diff --git a/Core/HW/SasAudio.cpp b/Core/HW/SasAudio.cpp index 408585c4ee..46a8f7385c 100644 --- a/Core/HW/SasAudio.cpp +++ b/Core/HW/SasAudio.cpp @@ -774,6 +774,11 @@ void SasInstance::DoState(PointerWrap &p) { Do(p, grainSize); if (p.mode == p.MODE_READ) { + if (grainSize > PSP_SAS_MAX_GRAIN) { + ERROR_LOG(Log::SaveState, "Bad SAS grain size %d", grainSize); + p.SetError(p.ERROR_FAILURE); + return; + } if (grainSize > 0) { SetGrainSize(grainSize); } else { @@ -803,6 +808,7 @@ void SasInstance::DoState(PointerWrap &p) { Do(p, n); if (n != PSP_SAS_VOICES_MAX) { ERROR_LOG(Log::SaveState, "Wrong number of SAS voices"); + p.SetError(p.ERROR_FAILURE); return; } DoArray(p, voices, ARRAY_SIZE(voices)); diff --git a/Core/MemMap.cpp b/Core/MemMap.cpp index b27fcec5b3..212aad6d1e 100644 --- a/Core/MemMap.cpp +++ b/Core/MemMap.cpp @@ -369,6 +369,10 @@ static void DoMemoryVoid(PointerWrap &p, uint32_t start, uint32_t size) { if ((size & 0x3F) != 0 || ((uintptr_t)d & 0x3F) != 0) return p.DoVoid(d, size); + if ((p.mode == PointerWrap::MODE_READ || p.mode == PointerWrap::MODE_VERIFY) && !p.CheckRead(size)) { + return; + } + switch (p.mode) { case PointerWrap::MODE_READ: ParallelMemcpy(&g_threadManager, d, storage, size); diff --git a/Core/RetroAchievements.cpp b/Core/RetroAchievements.cpp index 723252e21a..a8f982b414 100644 --- a/Core/RetroAchievements.cpp +++ b/Core/RetroAchievements.cpp @@ -902,6 +902,9 @@ void DoState(PointerWrap &p) { data_size = (uint32_t)(g_rcClient ? rc_client_progress_size(g_rcClient) : 0); } Do(p, data_size); + if (p.mode == PointerWrap::MODE_READ && !p.CheckRead(data_size)) { + return; + } if (data_size > 0) { uint8_t *buffer = new uint8_t[data_size];