Merge pull request #1434 from nicolasnoble/asan

nicolasnoble · web-flow · commit 99831496a649 · 2023-09-05T22:05:38.000-07:00
First pass at resolving various asan issues.
diff --git a/.github/workflows/linux-asan.yml b/.github/workflows/linux-asan.yml
@@ -0,0 +1,34 @@
+name: Linux CI asan
+
+on:
+  push:
+    branches:
+      - main
+  pull_request:
+
+jobs:
+  asan:
+    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 BUILD=Release
+          cp src/mips/openbios/openbios.bin .
+          make -C src/mips/openbios clean
+          make -C src/mips/tests -j 2 PCSX_TESTS=true BUILD=Release
+          cp ./openbios.bin src/mips/openbios/
+      - name: Test
+        run: |
+          xvfb-run ./pcsx-redux-tests
diff --git 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 *>(file->opaque);
+            fi->close();
             delete fi;
             file->opaque = nullptr;
         };
@@ -140,16 +141,26 @@ 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<Context *>(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) {
         for (unsigned i = 1; i <= disc.trackCount; i++) {
             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 +183,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/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<File> sbihandle;
+    IO<File> sbihandle(new UvFile(filename));
     char buffer[16];
 
-    sbihandle.setFile(new UvFile(filename));
+    if (sbihandle->failed()) return false;
     if (g_emulator->settings.get<Emulator::SettingFullCaching>()) {
         sbihandle.asA<UvFile>()->startCaching();
     }
-    if (sbihandle->failed()) return false;
 
     // init
     sbicount = 0;
diff --git 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);
diff --git 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
@@ -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
@@ -32,12 +32,7 @@ namespace PCSX {
 class PIOCart {
   public:
     PIOCart() : m_pal(this) {
-        m_detachedMemory = static_cast<uint8_t *>(calloc(64 * 1024, 1));
-        if (m_detachedMemory == NULL) {
-            g_system->message("%s", _("Error allocating memory!"));
-        } else {
-            memset(m_detachedMemory, 0xff, static_cast<size_t>(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<uint8_t *>(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
@@ -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
@@ -49,8 +49,7 @@ void PCSX::Resources::loadIcon(std::function<void(const uint8_t*, uint32_t)> pro
         info[i].offset = ico->read<uint32_t>();
     }
     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<const uint8_t*>(slice.data()), slice.size());
     }
 }
diff --git 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/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");
diff --git 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
@@ -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<Owned>(m_data).ptr = data;
-        std::get<Owned>(m_data).size = size;
+        m_data = Owned{size, data};
     }
     void borrow(const Slice &other, uint32_t from = 0, uint32_t amount = std::numeric_limits<uint32_t>::max()) {
         const uint8_t *ptr = static_cast<const uint8_t *>(other.data());
diff --git a/src/support/uvfile.cc b/src/support/uvfile.cc
@@ -110,7 +110,13 @@ void PCSX::UvThreadOp::startThread() {
             c_tick, c_tick);
         barrier.set_value();
         uv_run(&s_uvLoop, UV_RUN_DEFAULT);
-        uv_loop_close(&s_uvLoop);
+        uv_close(reinterpret_cast<uv_handle_t *>(&s_kicker), [](auto handle) {});
+        uv_close(reinterpret_cast<uv_handle_t *>(&s_timer), [](auto handle) {});
+        uv_close(reinterpret_cast<uv_handle_t *>(&s_curlTimeout), [](auto handle) {});
+        while (uv_loop_close(&s_uvLoop) == UV_EBUSY) {
+            uv_run(&s_uvLoop, UV_RUN_NOWAIT);
+        }
+        curl_multi_cleanup(s_curlMulti);
     });
     f.wait();
 }
@@ -184,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<uv_handle_t *>(&s_kicker), [](auto handle) {});
-        uv_close(reinterpret_cast<uv_handle_t *>(&s_timer), [](auto handle) {});
+        uv_unref(reinterpret_cast<uv_handle_t *>(&s_kicker));
+        uv_unref(reinterpret_cast<uv_handle_t *>(&s_timer));
+        uv_unref(reinterpret_cast<uv_handle_t *>(&s_curlTimeout));
     });
     s_uvThread.join();
     s_threadRunning = false;
@@ -266,7 +273,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
@@ -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 {
diff --git 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;
     }
 }
 

Original file line number	Diff line number	Diff line change
`@@ -74,6 +74,7 @@ class CallStacks {`
`74`	`74`	`m_currentSP = 0;`
`75`	`75`	`});`
`76`	`76`	`}`
	`77`	`+ ~CallStacks() { m_callstacks.destroyAll(); }`
`77`	`78`	`void setSP(uint32_t oldSP, uint32_t newSP);`
`78`	`79`	`void offsetSP(uint32_t oldSP, int32_t offset);`
`79`	`80`	`void storeRA(uint32_t sp, uint32_t ra);`
Original file line number	Diff line number	Diff line change
`@@ -49,8 +49,7 @@ void PCSX::Resources::loadIcon(std::function<void(const uint8_t*, uint32_t)> pro`
`49`	`49`	`info[i].offset = ico->read<uint32_t>();`
`50`	`50`	`}`
`51`	`51`	`for (unsigned i = 0; i < count; i++) {`
`52`		`- ico->rSeek(info[i].offset, SEEK_SET);`
`53`		`- auto slice = ico->read(info[i].size);`
	`52`	`+ auto slice = ico->readAt(info[i].size, info[i].offset);`
`54`	`53`	`process(reinterpret_cast<const uint8_t*>(slice.data()), slice.size());`
`55`	`54`	`}`
`56`	`55`	`}`