From f26f21d66cc7469cec7fe13e99aff1fccc0962e9 Mon Sep 17 00:00:00 2001 From: "Nicolas \"Pixel\" Noble" Date: Sun, 3 Sep 2023 23:41:57 -0700 Subject: [PATCH 01/12] First pass at resolving various asan issues. --- src/core/pad.h | 2 ++ src/core/pio-cart.cc | 12 ++++++------ src/core/pio-cart.h | 22 ++++++---------------- src/core/system.h | 2 +- src/gui/resources-unix.cc | 3 +-- src/main/main.cc | 2 +- src/spu/miniaudio.cc | 1 + src/support/slice.h | 4 +--- 8 files changed, 19 insertions(+), 29 deletions(-) diff --git a/src/core/pad.h b/src/core/pad.h index 4bd5c6468..ba0801a7a 100644 --- a/src/core/pad.h +++ b/src/core/pad.h @@ -34,6 +34,8 @@ class Pads { public: enum class Port { Port1 = 0, Port2 }; + virtual ~Pads() = default; + virtual void init() = 0; virtual void shutdown() = 0; virtual uint8_t startPoll(Port port) = 0; diff --git a/src/core/pio-cart.cc b/src/core/pio-cart.cc index 66bb6242a..26e0f124d 100644 --- a/src/core/pio-cart.cc +++ b/src/core/pio-cart.cc @@ -42,13 +42,13 @@ void PCSX::PIOCart::setLuts() { m_readLUT[0x1f05] = &m_exp1[1 << 16]; m_pal.reset(); } else { - memset(&m_readLUT[0x1f00], NULL, 0x6 * sizeof(void *)); + memset(&m_readLUT[0x1f00], 0, 0x6 * sizeof(void *)); } - // NULL by default, wipe to ensure writes are properly intercepted - memset(&m_writeLUT[0x1f00], NULL, 0x6 * sizeof(void *)); - memset(&m_writeLUT[0x9f00], NULL, 0x6 * sizeof(void *)); - memset(&m_writeLUT[0xbf00], NULL, 0x6 * sizeof(void *)); + // nullptr by default, wipe to ensure writes are properly intercepted + memset(&m_writeLUT[0x1f00], 0, 0x6 * sizeof(void *)); + memset(&m_writeLUT[0x9f00], 0, 0x6 * sizeof(void *)); + memset(&m_writeLUT[0xbf00], 0, 0x6 * sizeof(void *)); memcpy(&m_readLUT[0x9f00], &m_readLUT[0x1f00], 0x6 * sizeof(void *)); memcpy(&m_readLUT[0xbf00], &m_readLUT[0x1f00], 0x6 * sizeof(void *)); @@ -133,7 +133,7 @@ void PCSX::PIOCart::PAL::setLUTFlashBank(uint8_t bank) { const auto &m_readLUT = g_emulator->m_mem->m_readLUT; const auto &m_exp1 = g_emulator->m_mem->m_exp1; - if (m_readLUT == NULL || m_exp1 == NULL) return; + if (m_readLUT == nullptr || m_exp1 == nullptr) return; switch (bank) { case 0: diff --git a/src/core/pio-cart.h b/src/core/pio-cart.h index e6b359df4..1c735d736 100644 --- a/src/core/pio-cart.h +++ b/src/core/pio-cart.h @@ -32,12 +32,7 @@ namespace PCSX { class PIOCart { public: PIOCart() : m_pal(this) { - m_detachedMemory = static_cast(calloc(64 * 1024, 1)); - if (m_detachedMemory == NULL) { - g_system->message("%s", _("Error allocating memory!")); - } else { - memset(m_detachedMemory, 0xff, static_cast(64 * 1024)); - } + memset(m_detachedMemory, 0xff, sizeof(m_detachedMemory)); } void setLuts(); @@ -80,7 +75,7 @@ class PIOCart { private: bool m_switchOn = true; - uint8_t *m_detachedMemory = NULL; + uint8_t m_detachedMemory[64 * 1024]; class PAL { public: @@ -101,14 +96,9 @@ class PIOCart { class FlashMemory { public: FlashMemory(PAL *parent) : m_pal(parent) { - m_softwareID = static_cast(calloc(64 * 1024, 1)); - if (m_softwareID == NULL) { - g_system->message("%s", _("Error allocating memory!")); - } else { - for (int i = 0; i < (64 * 1024) - 1; i += 2) { - m_softwareID[i] = 0xbf; - m_softwareID[i + 1] = 0x10; - } + for (int i = 0; i < (64 * 1024) - 1; i += 2) { + m_softwareID[i] = 0xbf; + m_softwareID[i + 1] = 0x10; } } @@ -158,7 +148,7 @@ class PIOCart { uint8_t m_commandBuffer[m_bufferSize] = {0}; uint8_t m_busCycle = 0; - uint8_t *m_softwareID = NULL; + uint8_t m_softwareID[64 * 1024]; bool m_dataProtectEnabled = true; bool m_pageWriteEnabled = false; diff --git a/src/core/system.h b/src/core/system.h index a23e4cecb..15ca820d8 100644 --- a/src/core/system.h +++ b/src/core/system.h @@ -259,7 +259,7 @@ class System { protected: std::filesystem::path m_binDir; PCSX::VersionInfo m_version; - bool m_emergencyExit = true; + bool m_emergencyExit = false; }; extern System *g_system; diff --git a/src/gui/resources-unix.cc b/src/gui/resources-unix.cc index d82d6fc01..528bfcae8 100644 --- a/src/gui/resources-unix.cc +++ b/src/gui/resources-unix.cc @@ -49,8 +49,7 @@ void PCSX::Resources::loadIcon(std::function pro info[i].offset = ico->read(); } for (unsigned i = 0; i < count; i++) { - ico->rSeek(info[i].offset, SEEK_SET); - auto slice = ico->read(info[i].size); + auto slice = ico->readAt(info[i].size, info[i].offset); process(reinterpret_cast(slice.data()), slice.size()); } } diff --git a/src/main/main.cc b/src/main/main.cc index b74771180..d47f71ba4 100644 --- a/src/main/main.cc +++ b/src/main/main.cc @@ -359,9 +359,9 @@ int pcsxMain(int argc, char **argv) { emulator->m_spu->close(); emulator->m_cdrom->clearIso(); - emulator->m_cpu->psxShutdown(); emulator->m_spu->shutdown(); emulator->m_gpu->shutdown(); + emulator->shutdown(); s_ui->close(); delete s_ui; diff --git a/src/spu/miniaudio.cc b/src/spu/miniaudio.cc index 55d7c9b64..a7497fe90 100644 --- a/src/spu/miniaudio.cc +++ b/src/spu/miniaudio.cc @@ -162,6 +162,7 @@ void PCSX::SPU::MiniAudio::init(bool safe) { void PCSX::SPU::MiniAudio::uninit() { ma_device_uninit(&m_device); ma_device_uninit(&m_deviceNull); + ma_context_uninit(&m_context); } void PCSX::SPU::MiniAudio::maybeRestart() { diff --git a/src/support/slice.h b/src/support/slice.h index 615302ec1..db8ed37a9 100644 --- a/src/support/slice.h +++ b/src/support/slice.h @@ -121,9 +121,7 @@ class Slice { } void acquire(std::string &&str) { m_data = std::move(str); } void acquire(void *data, uint32_t size) { - m_data = Owned{size, malloc(size)}; - std::get(m_data).ptr = data; - std::get(m_data).size = size; + m_data = Owned{size, data}; } void borrow(const Slice &other, uint32_t from = 0, uint32_t amount = std::numeric_limits::max()) { const uint8_t *ptr = static_cast(other.data()); From 251001be13e93c9ff9d52193d3116c38d4b628e3 Mon Sep 17 00:00:00 2001 From: "Nicolas \"Pixel\" Noble" Date: Mon, 4 Sep 2023 08:30:44 -0700 Subject: [PATCH 02/12] Memory leak on CueFile objects. --- src/cdrom/cdriso-cue.cc | 22 +++++++++++++++++++--- third_party/cueparser/cueparser.c | 2 -- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/src/cdrom/cdriso-cue.cc b/src/cdrom/cdriso-cue.cc index 81086a294..1baaeb119 100644 --- a/src/cdrom/cdriso-cue.cc +++ b/src/cdrom/cdriso-cue.cc @@ -140,7 +140,15 @@ bool PCSX::CDRIso::parsecue(const char *isofileString) { }); Scheduler_run(&scheduler); - CueParser_destroy(&parser); + CueParser_close(&parser, &scheduler, [](CueParser *parser, CueScheduler *scheduler, const char *error) { + Context *context = reinterpret_cast(scheduler->opaque); + if (error) { + context->failed = true; + PCSX::g_system->printf("Error closing cue parser: %s", error); + } + CueParser_destroy(parser); + }); + Scheduler_run(&scheduler); File_schedule_close(&cue, &scheduler, [](CueFile *file, CueScheduler *scheduler) { file->destroy(file); }); if (context.failed) { @@ -148,8 +156,10 @@ bool PCSX::CDRIso::parsecue(const char *isofileString) { CueTrack *track = &disc.tracks[i]; if (track->file) { if (track->file->references == 1) { - File_schedule_close(track->file, &scheduler, - [](CueFile *file, CueScheduler *scheduler) { file->destroy(file); }); + File_schedule_close(track->file, &scheduler, [](CueFile *file, CueScheduler *scheduler) { + file->destroy(file); + free(file); + }); } else { track->file->references--; } @@ -172,6 +182,12 @@ bool PCSX::CDRIso::parsecue(const char *isofileString) { m_ti[i].start = IEC60908b::MSF(track->indices[1] + 150); m_ti[i].pregap = IEC60908b::MSF(track->indices[1] - track->indices[0]); m_ti[i].length = IEC60908b::MSF(track->size); + if (track->file->references == 1) { + free(track->file); + } else { + track->file->references--; + } + track->file = nullptr; } m_numtracks = disc.trackCount; diff --git a/third_party/cueparser/cueparser.c b/third_party/cueparser/cueparser.c index f090e3589..4632d6b0d 100644 --- a/third_party/cueparser/cueparser.c +++ b/third_party/cueparser/cueparser.c @@ -158,8 +158,6 @@ void CueParser_close(struct CueParser* parser, struct CueScheduler* scheduler, parser->currentFile->user = parser; parser->currentFile->close(parser->currentFile, scheduler, close_cb); } - } else { - parser->currentFile = NULL; } } From e00c4c4b7cc00417edcdfe5828214074ff135b18 Mon Sep 17 00:00:00 2001 From: "Nicolas \"Pixel\" Noble" Date: Mon, 4 Sep 2023 12:39:56 -0700 Subject: [PATCH 03/12] Some more leaks. --- src/cdrom/cdriso-cue.cc | 1 + src/cdrom/cdriso-sbi.cc | 5 ++--- src/support/uvfile.cc | 5 ++++- src/support/uvfile.h | 2 +- 4 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/cdrom/cdriso-cue.cc b/src/cdrom/cdriso-cue.cc index 1baaeb119..ec7fc4ff6 100644 --- a/src/cdrom/cdriso-cue.cc +++ b/src/cdrom/cdriso-cue.cc @@ -81,6 +81,7 @@ bool PCSX::CDRIso::parsecue(const char *isofileString) { file->opaque = fi; file->destroy = [](CueFile *file) { File *fi = reinterpret_cast(file->opaque); + fi->close(); delete fi; file->opaque = nullptr; }; diff --git a/src/cdrom/cdriso-sbi.cc b/src/cdrom/cdriso-sbi.cc index 1e834da97..f6deb047b 100644 --- a/src/cdrom/cdriso-sbi.cc +++ b/src/cdrom/cdriso-sbi.cc @@ -20,14 +20,13 @@ #include "cdrom/cdriso.h" bool PCSX::CDRIso::LoadSBI(const char *filename) { - IO sbihandle; + IO sbihandle(new UvFile(filename)); char buffer[16]; - sbihandle.setFile(new UvFile(filename)); + if (sbihandle->failed()) return false; if (g_emulator->settings.get()) { sbihandle.asA()->startCaching(); } - if (sbihandle->failed()) return false; // init sbicount = 0; diff --git a/src/support/uvfile.cc b/src/support/uvfile.cc index 5ce1c3be9..8badc713e 100644 --- a/src/support/uvfile.cc +++ b/src/support/uvfile.cc @@ -266,7 +266,10 @@ void PCSX::UvFile::openwrapper(const char *filename, int flags) { } m_handle = handle; m_size = size; - if (handle >= 0) m_failed = false; + if (handle >= 0) { + m_failed = false; + m_pendingCloseInfo = new PendingCloseInfo(); + } } PCSX::UvFile::UvFile(const char *filename) : File(RO_SEEKABLE), m_filename(filename) { diff --git a/src/support/uvfile.h b/src/support/uvfile.h index ef31789b8..d61b4177f 100644 --- a/src/support/uvfile.h +++ b/src/support/uvfile.h @@ -218,7 +218,7 @@ class UvFile : public File, public UvThreadOp { bool closePending = false; }; static void closeUVHandle(uv_file handle, uv_loop_s* loop, PendingCloseInfo* pendingCloseInfo); - PendingCloseInfo* m_pendingCloseInfo = new PendingCloseInfo(); + PendingCloseInfo* m_pendingCloseInfo = nullptr; }; class UvFifo : public File, public UvThreadOp { From c490077c3b12fb8c61a95fe5aa425208ed16a15b Mon Sep 17 00:00:00 2001 From: "Nicolas \"Pixel\" Noble" Date: Mon, 4 Sep 2023 18:07:17 -0700 Subject: [PATCH 04/12] Adding asan github workflow. --- .github/workflows/linux-asan.yml | 34 ++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 .github/workflows/linux-asan.yml diff --git a/.github/workflows/linux-asan.yml b/.github/workflows/linux-asan.yml new file mode 100644 index 000000000..af6b44a93 --- /dev/null +++ b/.github/workflows/linux-asan.yml @@ -0,0 +1,34 @@ +name: Linux CI asan + +on: + push: + branches: + - main + pull_request: + +jobs: + coverage: + runs-on: ubuntu-latest + container: + image: ghcr.io/grumpycoders/pcsx-redux-build:latest + env: + TEST_RESULTS: /tmp/test-results + BUILD: asan + steps: + - uses: actions/checkout@v3 + with: + submodules: recursive + set-safe-directory: true + - uses: n1hility/cancel-previous-runs@v2 + with: + token: ${{ secrets.GITHUB_TOKEN }} + - run: | + make -j 2 all pcsx-redux-tests + make -C src/mips/openbios -j 2 + cp src/mips/openbios/openbios.bin . + make -C src/mips/openbios clean + make -C src/mips/tests -j 2 PCSX_TESTS=true + cp ./openbios.bin src/mips/openbios/ + - name: Test + run: | + xvfb-run catchsegv ./pcsx-redux-tests From 9077731ca1768b6fe7b626736ca2b7fd9978a2e1 Mon Sep 17 00:00:00 2001 From: "Nicolas \"Pixel\" Noble" Date: Mon, 4 Sep 2023 18:35:30 -0700 Subject: [PATCH 05/12] Fixing openbios build when in asan mode. --- .github/workflows/linux-asan.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/linux-asan.yml b/.github/workflows/linux-asan.yml index af6b44a93..139fbd948 100644 --- a/.github/workflows/linux-asan.yml +++ b/.github/workflows/linux-asan.yml @@ -24,7 +24,7 @@ jobs: token: ${{ secrets.GITHUB_TOKEN }} - run: | make -j 2 all pcsx-redux-tests - make -C src/mips/openbios -j 2 + make -C src/mips/openbios -j 2 BUILD=SmallDebug cp src/mips/openbios/openbios.bin . make -C src/mips/openbios clean make -C src/mips/tests -j 2 PCSX_TESTS=true From 851518accab4df69bfbfdcdc4f8c0eee8ef38c56 Mon Sep 17 00:00:00 2001 From: "Nicolas \"Pixel\" Noble" Date: Mon, 4 Sep 2023 18:52:52 -0700 Subject: [PATCH 06/12] Plugging another leak. --- src/core/callstacks.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/core/callstacks.h b/src/core/callstacks.h index 6fb0195c2..ca6c7f8b5 100644 --- a/src/core/callstacks.h +++ b/src/core/callstacks.h @@ -74,6 +74,7 @@ class CallStacks { m_currentSP = 0; }); } + ~CallStacks() { m_callstacks.destroyAll(); } void setSP(uint32_t oldSP, uint32_t newSP); void offsetSP(uint32_t oldSP, int32_t offset); void storeRA(uint32_t sp, uint32_t ra); From 64e79380a599d5bf07b8f4c2412bbd06807fac05 Mon Sep 17 00:00:00 2001 From: Nicolas Noble Date: Mon, 4 Sep 2023 19:58:46 -0700 Subject: [PATCH 07/12] Trying to make asan work. --- .github/workflows/linux-asan.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/linux-asan.yml b/.github/workflows/linux-asan.yml index 139fbd948..82f1fcc15 100644 --- a/.github/workflows/linux-asan.yml +++ b/.github/workflows/linux-asan.yml @@ -31,4 +31,4 @@ jobs: cp ./openbios.bin src/mips/openbios/ - name: Test run: | - xvfb-run catchsegv ./pcsx-redux-tests + xvfb-run ./pcsx-redux-tests From af5fa50701bb04934125fc93015e136e982f1a9e Mon Sep 17 00:00:00 2001 From: Nicolas Noble Date: Mon, 4 Sep 2023 20:08:30 -0700 Subject: [PATCH 08/12] Properly naming asan job. --- .github/workflows/linux-asan.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/linux-asan.yml b/.github/workflows/linux-asan.yml index 82f1fcc15..961f72181 100644 --- a/.github/workflows/linux-asan.yml +++ b/.github/workflows/linux-asan.yml @@ -7,7 +7,7 @@ on: pull_request: jobs: - coverage: + asan: runs-on: ubuntu-latest container: image: ghcr.io/grumpycoders/pcsx-redux-build:latest From f5beb5102efd11543d9651a1d000f9eaad8f5f30 Mon Sep 17 00:00:00 2001 From: Nicolas Noble Date: Mon, 4 Sep 2023 20:43:35 -0700 Subject: [PATCH 09/12] Mismatched new[] / delete[]. --- src/main/main.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/main.h b/src/main/main.h index 6d984dc36..8a4f662f3 100644 --- a/src/main/main.h +++ b/src/main/main.h @@ -39,7 +39,7 @@ class MainInvoker { for (char** ptr = m_args; *ptr; ptr++) { free(*ptr); } - delete m_args; + delete[] m_args; } int invoke() { fprintf(stderr, "Starting PCSX-Redux test with arguments:\n"); From 918fb799db8bf7cf5bbdc056bd1d0a26f87bc87d Mon Sep 17 00:00:00 2001 From: "Nicolas \"Pixel\" Noble" Date: Mon, 4 Sep 2023 21:32:34 -0700 Subject: [PATCH 10/12] Let's not build mips tests using asan. --- .github/workflows/linux-asan.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/linux-asan.yml b/.github/workflows/linux-asan.yml index 961f72181..5c791727d 100644 --- a/.github/workflows/linux-asan.yml +++ b/.github/workflows/linux-asan.yml @@ -24,10 +24,10 @@ jobs: token: ${{ secrets.GITHUB_TOKEN }} - run: | make -j 2 all pcsx-redux-tests - make -C src/mips/openbios -j 2 BUILD=SmallDebug + make -C src/mips/openbios -j 2 BUILD=Release cp src/mips/openbios/openbios.bin . make -C src/mips/openbios clean - make -C src/mips/tests -j 2 PCSX_TESTS=true + make -C src/mips/tests -j 2 PCSX_TESTS=true BUILD=Release cp ./openbios.bin src/mips/openbios/ - name: Test run: | From b00fd5b83e4ce5cf75124ba2790c9c2318bf85c4 Mon Sep 17 00:00:00 2001 From: "Nicolas \"Pixel\" Noble" Date: Tue, 5 Sep 2023 20:51:26 -0700 Subject: [PATCH 11/12] Fixing curl memory leak. --- src/support/uvfile.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/support/uvfile.cc b/src/support/uvfile.cc index 8badc713e..207947945 100644 --- a/src/support/uvfile.cc +++ b/src/support/uvfile.cc @@ -110,6 +110,7 @@ void PCSX::UvThreadOp::startThread() { c_tick, c_tick); barrier.set_value(); uv_run(&s_uvLoop, UV_RUN_DEFAULT); + curl_multi_cleanup(s_curlMulti); uv_loop_close(&s_uvLoop); }); f.wait(); From 26f63102738fa0bf699223109388d678b1a4cbab Mon Sep 17 00:00:00 2001 From: "Nicolas \"Pixel\" Noble" Date: Tue, 5 Sep 2023 21:12:42 -0700 Subject: [PATCH 12/12] Different libuv thread shutdown strategy to avoid leaks. --- src/support/uvfile.cc | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/support/uvfile.cc b/src/support/uvfile.cc index 207947945..5e7ac4002 100644 --- a/src/support/uvfile.cc +++ b/src/support/uvfile.cc @@ -110,8 +110,13 @@ void PCSX::UvThreadOp::startThread() { c_tick, c_tick); barrier.set_value(); uv_run(&s_uvLoop, UV_RUN_DEFAULT); + uv_close(reinterpret_cast(&s_kicker), [](auto handle) {}); + uv_close(reinterpret_cast(&s_timer), [](auto handle) {}); + uv_close(reinterpret_cast(&s_curlTimeout), [](auto handle) {}); + while (uv_loop_close(&s_uvLoop) == UV_EBUSY) { + uv_run(&s_uvLoop, UV_RUN_NOWAIT); + } curl_multi_cleanup(s_curlMulti); - uv_loop_close(&s_uvLoop); }); f.wait(); } @@ -185,8 +190,9 @@ void PCSX::UvThreadOp::stopThread() { request([&barrier](auto loop) { barrier.set_value(); }); barrier.get_future().wait(); request([](auto loop) { - uv_close(reinterpret_cast(&s_kicker), [](auto handle) {}); - uv_close(reinterpret_cast(&s_timer), [](auto handle) {}); + uv_unref(reinterpret_cast(&s_kicker)); + uv_unref(reinterpret_cast(&s_timer)); + uv_unref(reinterpret_cast(&s_curlTimeout)); }); s_uvThread.join(); s_threadRunning = false;