Address CodeRabbit PR 2013 review feedback

nicolasnoble · nicolasnoble · commit d45e6835109d · 2026-04-19T21:06:39.000-07:00
- Lua dir filter now also skips empty names (ffi.string returns "" for
  a single NUL byte, so the ".","..'," check needed the empty case)
- Gap/System/Hidden hex edit opens with explicit SectorMode so CDRIsoFile
  doesn't misdetect sector size (RAW for gap/system, M1/M2_FORM1/M2_FORM2
  for hidden files based on their detected mode)
- Sector span now uses XA attribute Form 2 bit (0x1000) to select
  2324-byte sector size for Mode 2 Form 2 files, instead of assuming
  2048 everywhere
- readerListDir drops the redundant reader-&gt;open() check, verifies the
  target is a directory before returning children
- File replace is now a coroutine like extract: yields every 50ms to
  keep UI responsive, warns when replacement is larger than target,
  zero-pads the tail when smaller so stale bytes don't leak through
- ISO9660Reader constructor consolidates the PVD loop to also track
  the VD set terminator; new getVDEnd() returns the first sector past
  the terminator for accurate "Volume Descriptors" system region size
- FlatEntry gains a sectors field so gap detection uses the actual
  on-disc footprint rather than recomputing from logical size

Signed-off-by: Nicolas 'Pixel' Noble &lt;nicolas@nobis-crew.org&gt;
diff --git a/src/cdrom/iso9660-reader.cc b/src/cdrom/iso9660-reader.cc
@@ -25,9 +25,12 @@
 
 PCSX::ISO9660Reader::ISO9660Reader(std::shared_ptr<CDRIso> iso) : m_iso(iso) {
     unsigned pvdSector = 16;
+    bool foundPVD = false;
 
+    // Scan the full Volume Descriptor Set (LBA 16..terminator) to both find
+    // the PVD and record where the set actually ends.
     while (true) {
-        IO<File> pvdFile(new CDRIsoFile(iso, pvdSector++, 2048));
+        IO<File> pvdFile(new CDRIsoFile(iso, pvdSector, 2048));
         if (pvdFile->failed()) {
             m_failed = true;
             return;
@@ -36,22 +39,25 @@ PCSX::ISO9660Reader::ISO9660Reader(std::shared_ptr<CDRIso> iso) : m_iso(iso) {
         uint8_t vd[7];
         pvdFile->readAt(vd, 7, 0);
         if ((vd[1] != 'C') || (vd[2] != 'D') || (vd[3] != '0') || (vd[4] != '0') || (vd[5] != '1') || (vd[6] != 1)) {
-            m_failed = true;
+            if (!foundPVD) m_failed = true;
             return;
         }
 
         if (vd[0] == 255) {
-            m_failed = true;
+            // Terminator: the VD set ends at the sector after this one.
+            m_vdEnd = pvdSector + 1;
+            if (!foundPVD) m_failed = true;
             return;
         }
 
-        if (vd[0] != 1) continue;
-
-        ISO9660LowLevel::PVD pvd;
-        pvd.deserialize(pvdFile);
+        if (vd[0] == 1 && !foundPVD) {
+            ISO9660LowLevel::PVD pvd;
+            pvd.deserialize(pvdFile);
+            m_pvd = pvd;
+            foundPVD = true;
+        }
 
-        m_pvd = pvd;
-        break;
+        pvdSector++;
     }
 }
 
diff --git a/src/cdrom/iso9660-reader.h b/src/cdrom/iso9660-reader.h
@@ -44,10 +44,14 @@ class ISO9660Reader {
     std::vector<FullDirEntry> listAllEntriesFrom(const ISO9660LowLevel::DirEntry& entry);
     const ISO9660LowLevel::DirEntry& getRootDirEntry() { return m_pvd.get<ISO9660LowLevel::PVD_RootDir>(); }
     const ISO9660LowLevel::PVD& getPVD() { return m_pvd; }
+    // Returns the LBA just past the end of the volume descriptor set, i.e. the
+    // first sector after the VD terminator (type 255). Returns 17 if not found.
+    uint32_t getVDEnd() { return m_vdEnd; }
 
   private:
     std::shared_ptr<CDRIso> m_iso;
     bool m_failed = false;
+    uint32_t m_vdEnd = 17;
 
     std::optional<FullDirEntry> findEntry(const std::string_view& filename);
     ISO9660LowLevel::PVD m_pvd;
diff --git a/src/core/isoffi.lua b/src/core/isoffi.lua
@@ -93,7 +93,7 @@ local function createIsoReaderWrapper(isoReader)
             local result = {}
             for i = 0, count - 1 do
                 local name = ffi.string(C.dirEntryName(entries, i))
-                if name ~= '\0' and name ~= '\1' then
+                if name ~= '' and name ~= '\0' and name ~= '\1' then
                     table.insert(result, {
                         name = name,
                         lba = C.dirEntryLBA(entries, i),
diff --git a/src/core/luaiso.cc b/src/core/luaiso.cc
@@ -67,14 +67,9 @@ DirEntries* readerListDir(PCSX::ISO9660Reader* reader, const char* path) {
     if (path == nullptr || path[0] == '\0') {
         return new DirEntries{reader->listAllEntriesFrom(root)};
     }
-    auto* file = reader->open(path);
-    if (file->failed()) {
-        delete file;
-        return new DirEntries{};
-    }
-    delete file;
-    // Need to find the directory entry for the given path to list its contents.
-    // Walk the path manually using listAllEntriesFrom.
+    // Walk the path using listAllEntriesFrom. ISO9660 directory entries
+    // don't carry version suffixes (only files do), so exact match on each
+    // path component is correct for directory listing.
     auto parts = PCSX::StringsHelpers::split(std::string_view(path), "/");
     PCSX::ISO9660LowLevel::DirEntry current = root;
     for (auto& part : parts) {
@@ -89,6 +84,10 @@ DirEntries* readerListDir(PCSX::ISO9660Reader* reader, const char* path) {
         }
         if (!found) return new DirEntries{};
     }
+    // Only return children if the target is actually a directory.
+    if ((current.get<PCSX::ISO9660LowLevel::DirEntry_Flags>().value & 2) == 0) {
+        return new DirEntries{};
+    }
     return new DirEntries{reader->listAllEntriesFrom(current)};
 }
 
@@ -120,6 +119,11 @@ struct GapList {
     std::vector<GapEntry> gaps;
 };
 
+// XA attribute bits (CD-XA spec, stored big-endian in directory record).
+// Bit 11 (0x0800): Mode 2 Form 1 data file
+// Bit 12 (0x1000): Mode 2 Form 2 interleaved audio/video
+static constexpr uint16_t XA_ATTR_FORM2 = 0x1000;
+
 static void collectAllEntries(PCSX::ISO9660Reader* reader, const PCSX::ISO9660LowLevel::DirEntry& dir,
                               std::vector<std::pair<uint32_t, uint32_t>>& out) {
     auto entries = reader->listAllEntriesFrom(dir);
@@ -128,7 +132,9 @@ static void collectAllEntries(PCSX::ISO9660Reader* reader, const PCSX::ISO9660Lo
         if (name.size() == 1 && (name[0] == '\0' || name[0] == '\1')) continue;
         uint32_t lba = entry.get<PCSX::ISO9660LowLevel::DirEntry_LBA>();
         uint32_t size = entry.get<PCSX::ISO9660LowLevel::DirEntry_Size>();
-        uint32_t sectors = (size + 2047) / 2048;
+        uint16_t attribs = xa.get<PCSX::ISO9660LowLevel::DirEntry_XA_Attribs>();
+        uint32_t sectorSize = (attribs & XA_ATTR_FORM2) ? 2324 : 2048;
+        uint32_t sectors = (size + sectorSize - 1) / sectorSize;
         out.push_back({lba, sectors});
         bool isDir = (entry.get<PCSX::ISO9660LowLevel::DirEntry_Flags>().value & 2) != 0;
         if (isDir) collectAllEntries(reader, entry, out);
@@ -142,8 +148,9 @@ GapList* readerFindGaps(PCSX::ISO9660Reader* reader) {
     // Account for ISO9660 system structures
     allFiles.push_back({0, 16});  // License/system area
     auto& pvd = reader->getPVD();
+    uint32_t vdEnd = reader->getVDEnd();
+    allFiles.push_back({16, vdEnd > 16 ? vdEnd - 16 : 1});  // Volume descriptors including terminator
     uint32_t lPathLoc = pvd.get<PCSX::ISO9660LowLevel::PVD_LPathTableLocation>();
-    allFiles.push_back({16, lPathLoc > 16 ? lPathLoc - 16 : 1});  // Volume descriptors
     uint32_t pathTableSize = pvd.get<PCSX::ISO9660LowLevel::PVD_PathTableSize>();
     uint32_t pathTableSectors = (pathTableSize + 2047) / 2048;
     allFiles.push_back({lPathLoc, pathTableSectors});
diff --git a/src/gui/widgets/isobrowser.cc b/src/gui/widgets/isobrowser.cc
@@ -139,7 +139,12 @@ void PCSX::Widgets::IsoBrowser::collectFlatEntries(const ISO9660LowLevel::DirEnt
         uint32_t size = dirEntry.get<ISO9660LowLevel::DirEntry_Size>();
         auto fullPath = path.empty() ? filename : path + "/" + filename;
 
-        m_flatEntries.push_back({fullPath, lba, size, isDir ? FlatEntry::Directory : FlatEntry::File, dirEntry});
+        // Form 2 files use 2324-byte data sectors; everything else logical uses 2048.
+        uint16_t xaAttribs = xa.get<ISO9660LowLevel::DirEntry_XA_Attribs>();
+        uint32_t sectorSize = (xaAttribs & 0x1000) ? 2324 : 2048;
+        uint32_t sectors = (size + sectorSize - 1) / sectorSize;
+        m_flatEntries.push_back(
+            {fullPath, lba, size, sectors, isDir ? FlatEntry::Directory : FlatEntry::File, dirEntry});
         if (isDir) collectFlatEntries(dirEntry, fullPath);
     }
 }
@@ -155,7 +160,7 @@ void PCSX::Widgets::IsoBrowser::scanGapSectors(std::vector<FlatEntry>& out, uint
             // Read failed, treat rest as gap
             uint32_t remaining = end - lba;
             auto label = fmt::format(f_("<gap {} sectors>"), remaining);
-            out.push_back({label, lba, remaining * 2352, FlatEntry::Gap, {}});
+            out.push_back({label, lba, remaining * 2352, remaining, FlatEntry::Gap, {}});
             break;
         }
 
@@ -173,7 +178,7 @@ void PCSX::Widgets::IsoBrowser::scanGapSectors(std::vector<FlatEntry>& out, uint
             }
             uint32_t count = lba - gapStart;
             auto label = fmt::format(f_("<gap {} sectors>"), count);
-            out.push_back({label, gapStart, count * 2352, FlatEntry::Gap, {}});
+            out.push_back({label, gapStart, count * 2352, count, FlatEntry::Gap, {}});
             continue;
         }
 
@@ -188,7 +193,7 @@ void PCSX::Widgets::IsoBrowser::scanGapSectors(std::vector<FlatEntry>& out, uint
             }
             uint32_t count = lba - fileStart;
             auto label = fmt::format(f_("<hidden M1 {} sectors>"), count);
-            out.push_back({label, fileStart, count * 2048, FlatEntry::HiddenM1, {}});
+            out.push_back({label, fileStart, count * 2048, count, FlatEntry::HiddenM1, {}});
             continue;
         }
 
@@ -219,7 +224,7 @@ void PCSX::Widgets::IsoBrowser::scanGapSectors(std::vector<FlatEntry>& out, uint
             uint32_t dataSize = isForm2 ? count * 2324 : count * 2048;
             auto label = fmt::format(f_("<hidden {} f={} ch={} {} sectors>"),
                                      isForm2 ? "M2F2" : "M2F1", fileNum, channelNum, count);
-            out.push_back({label, fileStart, dataSize, type, {}});
+            out.push_back({label, fileStart, dataSize, count, type, {}});
             continue;
         }
 
@@ -234,30 +239,36 @@ void PCSX::Widgets::IsoBrowser::drawFilesystemFlat() {
 
         // Add ISO9660 system structures
         auto& pvd = m_reader->getPVD();
-        m_flatEntries.push_back({_("<License/System Area>"), 0, 16 * 2352, FlatEntry::System, {}});
-        m_flatEntries.push_back({_("<Volume Descriptors>"), 16, 2048, FlatEntry::System, {}});
-        // End volume descriptor at sector 17 (minimum), but path table location tells us where it ends
+        uint32_t vdEnd = m_reader->getVDEnd();
+        m_flatEntries.push_back({_("<License/System Area>"), 0, 16 * 2352, 16, FlatEntry::System, {}});
+        // Volume descriptor set spans from LBA 16 up to (but not including) vdEnd,
+        // including the PVD, any SVDs, and the terminator.
+        uint32_t vdSectors = vdEnd > 16 ? vdEnd - 16 : 1;
+        m_flatEntries.push_back(
+            {_("<Volume Descriptors>"), 16, vdSectors * 2048, vdSectors, FlatEntry::System, {}});
         uint32_t lPathLoc = pvd.get<ISO9660LowLevel::PVD_LPathTableLocation>();
-        if (lPathLoc > 17) {
-            m_flatEntries.push_back({_("<Volume Descriptors cont.>"), 17, (lPathLoc - 17) * 2048, FlatEntry::System, {}});
-        }
         uint32_t pathTableSize = pvd.get<ISO9660LowLevel::PVD_PathTableSize>();
         uint32_t pathTableSectors = (pathTableSize + 2047) / 2048;
-        m_flatEntries.push_back({_("<L Path Table>"), lPathLoc, pathTableSize, FlatEntry::System, {}});
+        m_flatEntries.push_back(
+            {_("<L Path Table>"), lPathLoc, pathTableSize, pathTableSectors, FlatEntry::System, {}});
         uint32_t lPathOptLoc = pvd.get<ISO9660LowLevel::PVD_LPathTableOptLocation>();
         if (lPathOptLoc != 0) {
-            m_flatEntries.push_back({_("<L Path Table (opt)>"), lPathOptLoc, pathTableSize, FlatEntry::System, {}});
+            m_flatEntries.push_back(
+                {_("<L Path Table (opt)>"), lPathOptLoc, pathTableSize, pathTableSectors, FlatEntry::System, {}});
         }
         uint32_t mPathLoc = pvd.get<ISO9660LowLevel::PVD_MPathTableLocation>();
-        m_flatEntries.push_back({_("<M Path Table>"), mPathLoc, pathTableSize, FlatEntry::System, {}});
+        m_flatEntries.push_back(
+            {_("<M Path Table>"), mPathLoc, pathTableSize, pathTableSectors, FlatEntry::System, {}});
         uint32_t mPathOptLoc = pvd.get<ISO9660LowLevel::PVD_MPathTableOptLocation>();
         if (mPathOptLoc != 0) {
-            m_flatEntries.push_back({_("<M Path Table (opt)>"), mPathOptLoc, pathTableSize, FlatEntry::System, {}});
+            m_flatEntries.push_back(
+                {_("<M Path Table (opt)>"), mPathOptLoc, pathTableSize, pathTableSectors, FlatEntry::System, {}});
         }
         auto& rootDir = m_reader->getRootDirEntry();
         uint32_t rootLBA = rootDir.get<ISO9660LowLevel::DirEntry_LBA>();
         uint32_t rootSize = rootDir.get<ISO9660LowLevel::DirEntry_Size>();
-        m_flatEntries.push_back({_("<Root Directory>"), rootLBA, rootSize, FlatEntry::System, {}});
+        uint32_t rootSectors = (rootSize + 2047) / 2048;
+        m_flatEntries.push_back({_("<Root Directory>"), rootLBA, rootSize, rootSectors, FlatEntry::System, {}});
 
         collectFlatEntries(m_reader->getRootDirEntry(), "");
         std::sort(m_flatEntries.begin(), m_flatEntries.end(),
@@ -270,11 +281,10 @@ void PCSX::Widgets::IsoBrowser::drawFilesystemFlat() {
             if (entry.lba > nextExpected) {
                 uint32_t gapSectors = entry.lba - nextExpected;
                 auto label = fmt::format(f_("<gap {} sectors>"), gapSectors);
-                withGaps.push_back({label, nextExpected, gapSectors * 2352, FlatEntry::Gap, {}});
+                withGaps.push_back({label, nextExpected, gapSectors * 2352, gapSectors, FlatEntry::Gap, {}});
             }
             withGaps.push_back(entry);
-            uint32_t sectors = (entry.size + 2047) / 2048;
-            uint32_t end = entry.lba + sectors;
+            uint32_t end = entry.lba + entry.sectors;
             if (end > nextExpected) nextExpected = end;
         }
         m_flatEntries = std::move(withGaps);
@@ -349,7 +359,16 @@ headers and subheader file boundary markers.)"));
                 if (ImGui::MenuItem(_("Hex Edit"))) {
                     auto isoPtr = m_cachedIso.lock();
                     if (isoPtr) {
-                        openHexEditor(entry.path, IO<File>(new CDRIsoFile(isoPtr, entry.lba, entry.size)));
+                        IEC60908b::SectorMode mode = IEC60908b::SectorMode::GUESS;
+                        switch (entry.type) {
+                            case FlatEntry::Gap:
+                            case FlatEntry::System: mode = IEC60908b::SectorMode::RAW; break;
+                            case FlatEntry::HiddenM1: mode = IEC60908b::SectorMode::M1; break;
+                            case FlatEntry::HiddenM2F1: mode = IEC60908b::SectorMode::M2_FORM1; break;
+                            case FlatEntry::HiddenM2F2: mode = IEC60908b::SectorMode::M2_FORM2; break;
+                            default: break;
+                        }
+                        openHexEditor(entry.path, IO<File>(new CDRIsoFile(isoPtr, entry.lba, entry.size, mode)));
                     }
                 }
                 ImGui::EndPopup();
@@ -563,20 +582,55 @@ significantly by caching the files beforehand.)"));
                 uint32_t originalSize = m_selectedSize;
                 auto isoPtr = m_cachedIso.lock();
                 if (isoPtr) {
-                    IO<File> replacement(new UvFile(srcPath));
-                    if (!replacement->failed()) {
-                        IO<File> isoFile(new CDRIsoFile(isoPtr, lba, originalSize));
+                    m_extractionProgress = 0.0f;
+                    m_extractionCoroutine = [](IsoBrowser* self, std::shared_ptr<CDRIso> iso, uint32_t lba,
+                                               uint32_t originalSize, std::string src) -> Coroutine<> {
+                        auto time = std::chrono::steady_clock::now();
+                        IO<File> replacement(new UvFile(src));
+                        if (replacement->failed()) co_return;
+                        IO<File> isoFile(new CDRIsoFile(iso, lba, originalSize));
                         uint32_t replaceSize = std::min((uint32_t)replacement->size(), originalSize);
+                        if (replacement->size() > originalSize) {
+                            // Replacement too large; truncated to original size.
+                            g_system->printf(
+                                _("ISO replace: replacement file is larger than target (%zu > %u). Truncating.\n"),
+                                replacement->size(), originalSize);
+                        }
                         uint8_t buffer[2048];
                         uint32_t remaining = replaceSize;
+                        uint32_t written = 0;
                         while (remaining > 0) {
                             uint32_t chunk = std::min(remaining, (uint32_t)sizeof(buffer));
                             auto read = replacement->read(buffer, chunk);
                             if (read <= 0) break;
                             isoFile->write(buffer, read);
                             remaining -= read;
+                            written += read;
+                            if (std::chrono::steady_clock::now() - time > std::chrono::milliseconds(50)) {
+                                self->m_extractionProgress = (float)written / (float)originalSize;
+                                co_yield self->m_extractionCoroutine.awaiter();
+                                time = std::chrono::steady_clock::now();
+                            }
                         }
-                    }
+                        // Zero-pad the tail if the replacement was smaller than the original,
+                        // so stale bytes from the original don't leak through.
+                        if (written < originalSize) {
+                            uint8_t zeros[2048] = {0};
+                            uint32_t padRemaining = originalSize - written;
+                            while (padRemaining > 0) {
+                                uint32_t chunk = std::min(padRemaining, (uint32_t)sizeof(zeros));
+                                isoFile->write(zeros, chunk);
+                                padRemaining -= chunk;
+                                written += chunk;
+                                if (std::chrono::steady_clock::now() - time > std::chrono::milliseconds(50)) {
+                                    self->m_extractionProgress = (float)written / (float)originalSize;
+                                    co_yield self->m_extractionCoroutine.awaiter();
+                                    time = std::chrono::steady_clock::now();
+                                }
+                            }
+                        }
+                        self->m_extractionProgress = 1.0f;
+                    }(this, isoPtr, lba, originalSize, srcPath);
                 }
             }
         }
diff --git a/src/gui/widgets/isobrowser.h b/src/gui/widgets/isobrowser.h
@@ -80,6 +80,7 @@ class IsoBrowser {
         std::string path;
         uint32_t lba;
         uint32_t size;
+        uint32_t sectors;  // Actual sector span on disc, derived from XA form for Form 2 files.
         Type type;
         ISO9660LowLevel::DirEntry dirEntry;
 
