Isobrowser's filesystem#2013
Merged
nicolasnoble merged 18 commits intoApr 20, 2026
Merged
Conversation
The ISO browser widget previously only showed disc-level metadata (tracks, CRCs, game ID). This adds actual filesystem browsing by connecting the existing ISO9660Reader to the widget's UI: - Directory tree with Name/LBA/Size columns - File extraction to host filesystem via save dialog - File replacement through the PPF patching pipeline - PPF management controls (clear/save) The ISO9660Reader API is expanded to expose directory enumeration and root directory access, which were previously private. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
Exposes ISO9660 filesystem enumeration through the Lua FFI so scripts can list directory contents without the GUI. The reader:listDir(path) method returns a table of entries with name, LBA, size, and isDir fields, enabling headless ISO inspection and processing workflows. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
Adds a "Flat view (by sector)" toggle that shows all files and directories in a single table sorted by LBA, giving a disc topology view. Useful for spotting sector layout, gaps between files, and understanding the physical organization of the disc image. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
The flat view now shows gap entries between files, representing unallocated sectors on the disc. Gaps are selectable and can be extracted or replaced just like files, enabling inspection and patching of hidden or unused disc regions. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
Integrates the memory editor widget into the ISO browser. Selecting a file or gap and clicking "Hex Edit" opens a hex viewer/editor backed by a CDRIsoFile. Writes go through the PPF patching pipeline, so edits are non-destructive to the original disc image. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
Adds a "Scan gaps for hidden files" button in the flat view that reads sector headers in gap regions to detect content stripped from the ISO9660 directory. The scanner classifies sectors by mode: - Mode 0: true gap (empty/unused sectors) - Mode 1: hidden data (M1) - Mode 2: uses XA subheader to determine Form 1 vs Form 2, groups sectors into subfiles by file/channel number, and detects file boundaries via the end-of-file submode flag The scan is opt-in to avoid hammering the ISO on every flat view refresh. Results show the sector type (M1/M2F1/M2F2/Gap) and for Mode 2 files, the file number and channel from the subheader. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
Returns an array of {lba, sectors} tables for LBA ranges not covered
by any ISO9660 directory entry. Scripts can then use iso:open(lba,
-1, 'GUESS') on each gap to auto-detect hidden files via the
CDRIsoFile constructor's existing mode/subheader analysis.
Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
The flat view and Lua gap finder now include ISO9660 system area entries (license sectors 0-15, volume descriptors, L/M path tables, root directory record) so these regions are no longer reported as gaps. The PVD is exposed via ISO9660Reader::getPVD() to read path table locations and sizes. Also adds a System entry type to the flat view display. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
The tree and flat view tables now use the remaining window height instead of a fixed 300px, reserving space for the PPF buttons below. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
The ISO hex editor now uses mono font, data preview panel, and lowercase hex display, matching the appearance of the main memory editors. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
Extract/Replace/Hex Edit are now in a right-click context menu on file entries in both tree and flat views, replacing the toolbar buttons. Hex editors use an intrusive list so multiple files can be open simultaneously. Each HexEditorInstance holds its own MemoryEditor, IO<File>, and open state. When the user closes a hex editor window, the instance unlinks from the list and deletes itself on the next frame. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
Contributor
📝 WalkthroughWalkthroughExpose ISO9660 reader internals and VD-end, add Lua FFI for directory listing and gap discovery with C++ backends, and implement a full ISO filesystem browser UI (tree/flat views, gap scanning, extract/replace, hex editor) with GUI wiring and coroutines. Changes
sequenceDiagram
participant User as User (GUI)
participant IB as IsoBrowser (GUI)
participant LuaFFI as Lua FFI
participant Backend as C++ Backend
participant ISO as ISO9660Reader
participant FileIO as File I/O
participant Hex as Hex Editor
User->>IB: Open ISO / select path / toggle view
IB->>LuaFFI: readerListDir(path)
LuaFFI->>Backend: readerListDir(reader, path)
Backend->>ISO: traverse directory records
ISO-->>Backend: Dir entries
Backend-->>LuaFFI: DirEntries
LuaFFI-->>IB: entries (Lua table)
User->>IB: Scan gaps
IB->>LuaFFI: readerFindGaps()
LuaFFI->>Backend: readerFindGaps(reader)
Backend->>ISO: read VD/path tables + traverse tree
ISO-->>Backend: occupied intervals
Backend->>Backend: compute gaps
Backend-->>LuaFFI: GapList
LuaFFI-->>IB: gap entries (Lua table)
User->>IB: Extract / Open in hex
IB->>FileIO: stream extract (coroutine)
FileIO-->>IB: temp file handle
IB->>Hex: openHexEditor(title, file)
Hex-->>User: display file
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/isoffi.lua`:
- Around line 94-106: The loop is letting the "." entry through because
ffi.string(ptr) returns "" for a single NUL byte; update the filter in the loop
that processes dirEntryName(entries, i) to also exclude empty strings (name ==
"") in addition to '\0' and '\1'. Locate the loop using variables name, entries,
count and the C helpers dirEntryName/dirEntryLBA/dirEntrySize/dirEntryIsDir and
change the condition to skip entries where name == "" or name == '\0' or name ==
'\1' so the "." and ".." sentinel entries are both filtered out.
In `@src/core/luaiso.cc`:
- Around line 138-174: readerFindGaps currently assumes volume descriptors run
contiguously up to lPathLoc and that all files use 2048-byte sectors; update
readerFindGaps and collectAllEntries to compute true extents: in readerFindGaps
use the actual VD region end by iterating volume descriptors starting at LBA 16
until the VD terminator (so compute the span based on descriptor types rather
than lPathLoc heuristics referenced by lPathLoc and the initial
allFiles.push_back for volume descriptors), and in collectAllEntries/where file
sector counts are computed replace the hardcoded divisor (2048) with a
sectorSize derived from the XA/mode descriptor (use the XA/mode value already
available from listAllEntriesFrom destructuring) and compute sectors as (size +
sectorSize - 1) / sectorSize so Mode 2 Form 2 (e.g., 2324) files get the correct
footprint.
- Around line 65-93: Remove the redundant reader->open() check in readerListDir
and instead resolve the target entry using the reader's normalization logic (use
reader->findEntry(path) or equivalent) so ISO9660 version suffixes like ";1" are
handled; if path is null/empty use root, else call reader->findEntry(path) and
return new DirEntries{} when not found; before calling
reader->listAllEntriesFrom(...) verify the resolved
PCSX::ISO9660LowLevel::DirEntry is actually a directory (check its
type/isDirectory field) and return a distinct error/empty indicator if it's not
a directory so callers can distinguish "not found" vs "not a directory".
In `@src/gui/widgets/isobrowser.cc`:
- Around line 558-582: The replace path in the m_openReplaceFileDialog handler
leaves trailing original bytes when replacement->size() < originalSize and
blocks the UI thread; update the logic in the replacement branch (the code
creating IO<File> replacement and IO<File> isoFile via CDRIsoFile, computing
replaceSize) to (1) compute replaceSize and if replacement->size() >
originalSize surface a user-visible warning about truncation, and if
replacement->size() < originalSize write the replacement bytes then explicitly
zero-pad the remainder up to originalSize (instead of leaving stale tail bytes),
(2) detect and report open/read/write errors via the existing UI notification
mechanism when replacement->failed() or read/write return short, and (3) move
the read/write loop into the asynchronous Coroutine<> pattern used by the
extract path so the operation yields to the frame loop and does not block the UI
thread. Ensure references to m_selectedLBA/m_selectedSize and CDRIsoFile are
preserved when capturing state into the coroutine.
- Around line 349-354: The hex editor call in isobrowser.cc passes gap
entry.size directly into CDRIsoFile which causes misreported sector sizes
because CDRIsoFile may detect a different sector mode; update the openHexEditor
invocation that constructs CDRIsoFile (the call inside the ImGui::MenuItem("Hex
Edit") block) to explicitly open gap regions as raw 2352-byte sectors by
supplying SectorMode::RAW (or alternatively stop passing an explicit byte size
and let CDRIsoFile handle detection consistently) so sector boundaries shown by
the hex editor match the intended interpretation of gap entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5a29c3ab-946b-454a-84a1-67ef9d3c105c
📒 Files selected for processing (6)
src/cdrom/iso9660-reader.hsrc/core/isoffi.luasrc/core/luaiso.ccsrc/gui/gui.ccsrc/gui/widgets/isobrowser.ccsrc/gui/widgets/isobrowser.h
- 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->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 <nicolas@nobis-crew.org>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/luaiso.cc (1)
22-30:⚠️ Potential issue | 🟡 MinorInclude
<algorithm>wherestd::sortis used.This file calls
std::sort()on line 169 but does not directly include<algorithm>. While the header is available transitively throughsupport/strings-helpers.h, explicit inclusion should be added to avoid relying on transitive dependencies.Suggested fix
`#include` "core/luaiso.h" +#include <algorithm> `#include` <memory>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/luaiso.cc` around lines 22 - 30, The file luaiso.cc uses std::sort (call site in this translation unit) but doesn't include <algorithm> directly; add an explicit `#include` <algorithm> to the top of src/core/luaiso.cc (alongside the other headers) so std::sort is provided without relying on transitive includes from support/strings-helpers.h and ensure the translation unit compiles robustly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cdrom/iso9660-reader.cc`:
- Around line 41-49: The code returns early when a later volume descriptor has
an invalid CD001 signature while foundPVD is true, leaving m_vdEnd at its
default and allowing those sectors to be treated as free; update the
failing-signature branch in the loop that checks vd[] (the block that currently
does "if ((vd[1] != 'C') ... ) { if (!foundPVD) m_failed = true; return; }") so
that when foundPVD is true you set m_vdEnd = pvdSector before returning (keep
the existing m_failed behavior when !foundPVD unchanged) to ensure downstream
gap/flat-layout logic does not reuse those descriptor sectors.
In `@src/core/luaiso.cc`:
- Around line 171-180: readerFindGaps currently only records gaps before the
next occupied extent and never emits the trailing gap after the last occupied
interval; after the existing loop that builds result->gaps using allFiles and
nextExpected, fetch the disc/volume length exposed by the reader (e.g., use the
ISO volume size or the reader's disc length accessor available in this module)
and if that end value is greater than nextExpected push a gap {nextExpected, end
- nextExpected} onto result->gaps so appended/removed content after the last
extent is reported to Lua callers.
- Around line 65-68: readerListDir currently calls reader->getRootDirEntry()
without checking whether the ISO9660Reader succeeded; mirror the guard used in
readerFindGaps by checking the reader's success/state before touching PVD data
and return an empty DirEntries early on failure. Modify readerListDir to
validate the reader (same predicate used in readerFindGaps) and if it indicates
a failed reader, return an empty/new DirEntries immediately instead of calling
getRootDirEntry()/listAllEntriesFrom.
In `@src/gui/widgets/isobrowser.cc`:
- Around line 295-309: The UI currently performs gap scanning synchronously
inside the ImGui button handler (the block using ImGui::Button(_("Scan gaps for
hidden files")), iterating m_flatEntries and calling scanGapSectors), which can
block the UI; refactor this to spawn the existing Coroutine<>/progress workflow
used by CRC/extraction instead: when the button is pressed, start a Coroutine
that iterates m_flatEntries, calls scanGapSectors for gaps (accumulating scanned
entries into a local vector), updates progress via the same progress reporting
mechanism, and on completion assigns m_flatEntries = std::move(scanned) and sets
m_gapsScanned = true; ensure the coroutine captures m_cachedIso.lock() safely
and cancels/handles errors consistent with other coroutines.
- Around line 277-290: The flat-view gap insertion loop misses a final gap after
the last known entry; compute the disc end LBA via iso->getTD(0).toLBA() (or
equivalent), and if nextExpected < disc_end_lba push one more FlatEntry of type
FlatEntry::Gap spanning from nextExpected to (disc_end_lba - nextExpected)
sectors (and size = sectors * 2352, label like "<gap N sectors>") before
assigning m_flatEntries = std::move(withGaps); update the loop in isobrowser.cc
around the m_flatEntries population so the appended/orphaned sectors at image
end are represented.
- Around line 331-342: The selected-state calculation only compares
m_selectedIsGap to entry.isGap(), so rows where entry.isHidden() are not kept
highlighted; change the selected boolean to compare m_selectedIsGap against the
combined gap/hidden predicate — e.g. set selected = m_hasSelection &&
m_selectedLBA == entry.lba && m_selectedIsGap == (entry.isGap() ||
entry.isHidden()) — since the selection setter already assigns m_selectedIsGap =
entry.isGap() || entry.isHidden(); update the comparison to use entry.isHidden()
alongside entry.isGap() (referencing m_hasSelection, m_selectedLBA,
m_selectedIsGap, entry.lba, entry.isGap(), and entry.isHidden()).
---
Outside diff comments:
In `@src/core/luaiso.cc`:
- Around line 22-30: The file luaiso.cc uses std::sort (call site in this
translation unit) but doesn't include <algorithm> directly; add an explicit
`#include` <algorithm> to the top of src/core/luaiso.cc (alongside the other
headers) so std::sort is provided without relying on transitive includes from
support/strings-helpers.h and ensure the translation unit compiles robustly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 191138ee-4b5e-499a-8498-9656c5f4820c
📒 Files selected for processing (6)
src/cdrom/iso9660-reader.ccsrc/cdrom/iso9660-reader.hsrc/core/isoffi.luasrc/core/luaiso.ccsrc/gui/widgets/isobrowser.ccsrc/gui/widgets/isobrowser.h
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/isoffi.lua
- src/cdrom/iso9660-reader.h
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2013 +/- ##
==========================================
- Coverage 10.27% 10.25% -0.03%
==========================================
Files 519 519
Lines 149850 150329 +479
==========================================
+ Hits 15397 15410 +13
- Misses 134453 134919 +466 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- ISO9660Reader: malformed descriptor after the PVD now fails the reader instead of returning early with a stale m_vdEnd default. - readerListDir: guard against failed readers up front, matching readerFindGaps. - readerFindGaps: emit a trailing gap from the last occupied extent to the end of the disc (PVD VolumeSpaceSize) so appended/orphaned content is visible to Lua callers. - Flat view: same trailing-gap addition so the GUI doesn't hide content past the last known file. - Flat view selection highlight: check isGap() || isHidden() to match how m_selectedIsGap is set, so M1/M2F1/M2F2 rows keep their selected state. - Gap scanning: moved to a Coroutine<> with progress bar; yields every 50ms so large images or trailing gaps no longer freeze the UI. The scan coroutine is reset on ISO change. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/gui/widgets/isobrowser.cc (3)
329-335:⚠️ Potential issue | 🟠 MajorUse the loaded disc end for the trailing gap.
PVD_VolumeSpaceSizeis the ISO9660 logical volume length, not necessarily the end of the loaded image. Appended/orphan sectors beyond the volume still won’t be represented; use the actual disc end shown elsewhere viaiso->getTD(0).toLBA().Proposed fix
// Trailing gap: append any unreferenced sectors at the end of the image. - uint32_t discEnd = pvd.get<ISO9660LowLevel::PVD_VolumeSpaceSize>(); + uint32_t discEnd = pvd.get<ISO9660LowLevel::PVD_VolumeSpaceSize>(); + if (auto iso = m_cachedIso.lock()) { + discEnd = iso->getTD(0).toLBA(); + } if (discEnd > nextExpected) { uint32_t gapSectors = discEnd - nextExpected; auto label = fmt::format(f_("<gap {} sectors>"), gapSectors);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/widgets/isobrowser.cc` around lines 329 - 335, The trailing-gap calculation uses pvd.get<ISO9660LowLevel::PVD_VolumeSpaceSize>() (stored in discEnd) which is the logical volume length, not the actual loaded image end; replace discEnd with the actual loaded disc end from iso->getTD(0).toLBA() when computing gapSectors and constructing the gap entry pushed to withGaps (keep nextExpected, gap size in bytes = gapSectors * 2352, and label creation via fmt::format the same). Locate the block that declares discEnd and computes gapSectors and change the source of discEnd to iso->getTD(0).toLBA(), preserving the rest of the logic that appends the FlatEntry gap to withGaps.
386-414:⚠️ Potential issue | 🟠 MajorUse the same sector mode for Extract/Replace as Hex Edit.
Flat entries have type-dependent size units: gaps are raw
2352 * sectors, while hidden files use logical M1/M2 sizes. Hex Edit handles this with an explicit mode switch, but Extract/Replace only persist LBA/size and later openCDRIsoFilewithGUESS, so right-click Extract/Replace on a gap can read/write the wrong sector span. Capture the selected sector mode and pass it into those coroutines too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/widgets/isobrowser.cc` around lines 386 - 414, Extract/Replace handlers currently set m_selectedPath/m_selectedLBA/m_selectedSize and open dialogs but don't store the IEC60908b::SectorMode used by Hex Edit, causing wrong size units for gaps/hidden files; modify the Extract and Replace branches to compute the same mode with the existing switch (using IEC60908b::SectorMode::RAW, M1, M2_FORM1, M2_FORM2, GUESS) and persist it (e.g., add m_selectedSectorMode or similar) alongside m_hasSelection, then ensure the save/replace coroutines and any place that constructs CDRIsoFile use that stored mode instead of GUESS when opening the file.
632-675:⚠️ Potential issue | 🟠 MajorAbort replacement on I/O errors instead of padding over them.
If
replacement->read()fails beforereplaceSizebytes are copied, the loop breaks and the zero-padding block overwrites the rest of the ISO target.isoFile->write()is also unchecked, andreplacement->size()is narrowed beforestd::min, which can mis-handle very large files.Proposed fix
auto time = std::chrono::steady_clock::now(); IO<File> replacement(new UvFile(src)); - if (replacement->failed()) co_return; + if (replacement->failed()) { + g_system->printf(_("ISO replace: failed to open replacement file.\n")); + co_return; + } IO<File> isoFile(new CDRIsoFile(iso, lba, originalSize)); - uint32_t replaceSize = std::min((uint32_t)replacement->size(), originalSize); - if (replacement->size() > originalSize) { + size_t replacementSize = replacement->size(); + uint32_t replaceSize = + replacementSize > originalSize ? originalSize : static_cast<uint32_t>(replacementSize); + if (replacementSize > 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); + replacementSize, 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 (read <= 0) { + g_system->printf(_("ISO replace: failed while reading replacement file.\n")); + co_return; + } + auto wrote = isoFile->write(buffer, read); + if (wrote != read) { + g_system->printf(_("ISO replace: failed while writing to ISO.\n")); + co_return; + } + remaining -= static_cast<uint32_t>(read); + written += static_cast<uint32_t>(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(); @@ 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; + auto wrote = isoFile->write(zeros, chunk); + if (wrote != static_cast<ssize_t>(chunk)) { + g_system->printf(_("ISO replace: failed while zero-padding ISO.\n")); + co_return; + } + padRemaining -= static_cast<uint32_t>(wrote); + written += static_cast<uint32_t>(wrote); if (std::chrono::steady_clock::now() - time > std::chrono::milliseconds(50)) { self->m_extractionProgress = (float)written / (float)originalSize; co_yield self->m_extractionCoroutine.awaiter();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/widgets/isobrowser.cc` around lines 632 - 675, The replacement loop currently treats read/write failures as normal termination and then zero-pads the rest; change this so any I/O error aborts the replacement (no padding) and surfaces an error. Specifically, compute replaceSize using a safe size_t-to-uint32_t clamp (use replacement->size() as size_t and cap to originalSize) instead of narrowing before std::min; after each replacement->read(...) check for negative/error and on error set an error flag and break/return without performing the zero-pad, and after each isoFile->write(...) verify the write succeeded (handle short/written<requested as an error). If an error occurs, ensure you stop the coroutine and report the failure (e.g., via m_extractionProgress or an error return) rather than continuing to pad; reference IO<File> replacement, CDRIsoFile isoFile, replacement->read, isoFile->write, replaceSize, written and m_extractionProgress to locate where to add these checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/luaiso.cc`:
- Around line 128-142: collectAllEntries must skip zero-length extents and avoid
directory cycles: stop pushing entries when sectors==0 (do not out.push_back for
{lba, 0}) so nextExpected can advance, and track visited directory LBAs to
prevent unbounded recursion. Modify collectAllEntries to accept a visited set
(e.g., std::unordered_set<uint32_t>& visitedDirs) or maintain one externally,
insert the directory LBA before recursing, and before recursing check if lba is
already in visitedDirs; only recurse if not visited. Use the existing symbols
reader, entry, lba, sectors and isDir to implement these checks and skip logic.
- Around line 65-93: The Lua-facing directory listing exposes ISO9660 special
entries '\0' and '\1' (self/parent) because reader->listAllEntriesFrom(...) is
returned directly; filter out any DirEntry whose
PCSX::ISO9660LowLevel::DirEntry_Filename().value is exactly "\0" or "\1" (or
whose first char is '\0' or '\1') before returning or when iterating. Update
readerListDir to drop those entries from the vectors produced by
listAllEntriesFrom (both when traversing parts and for the final return) so the
returned DirEntries contain no entries with filename value '\0' or '\1'.
In `@src/gui/widgets/isobrowser.cc`:
- Around line 265-266: The loop that handles gap scanning currently increments
lba and scannedSectors for unrecognized sector modes without emitting any
FlatEntry or performing the coroutine yield check, which drops bytes from the
flat view and can freeze the UI; update the branch where only lba++ and
scannedSectors++ are executed to instead create and append a FlatEntry
representing the unrecognized sector (populate mode/size/offset minimally so the
bytes remain visible in the flat view), increment lba and scannedSectors as
before, and invoke the existing coroutine/yield check used elsewhere in this
scanner (call the same yield/check function used in other scan branches) so long
runs yield to the UI.
---
Duplicate comments:
In `@src/gui/widgets/isobrowser.cc`:
- Around line 329-335: The trailing-gap calculation uses
pvd.get<ISO9660LowLevel::PVD_VolumeSpaceSize>() (stored in discEnd) which is the
logical volume length, not the actual loaded image end; replace discEnd with the
actual loaded disc end from iso->getTD(0).toLBA() when computing gapSectors and
constructing the gap entry pushed to withGaps (keep nextExpected, gap size in
bytes = gapSectors * 2352, and label creation via fmt::format the same). Locate
the block that declares discEnd and computes gapSectors and change the source of
discEnd to iso->getTD(0).toLBA(), preserving the rest of the logic that appends
the FlatEntry gap to withGaps.
- Around line 386-414: Extract/Replace handlers currently set
m_selectedPath/m_selectedLBA/m_selectedSize and open dialogs but don't store the
IEC60908b::SectorMode used by Hex Edit, causing wrong size units for gaps/hidden
files; modify the Extract and Replace branches to compute the same mode with the
existing switch (using IEC60908b::SectorMode::RAW, M1, M2_FORM1, M2_FORM2,
GUESS) and persist it (e.g., add m_selectedSectorMode or similar) alongside
m_hasSelection, then ensure the save/replace coroutines and any place that
constructs CDRIsoFile use that stored mode instead of GUESS when opening the
file.
- Around line 632-675: The replacement loop currently treats read/write failures
as normal termination and then zero-pads the rest; change this so any I/O error
aborts the replacement (no padding) and surfaces an error. Specifically, compute
replaceSize using a safe size_t-to-uint32_t clamp (use replacement->size() as
size_t and cap to originalSize) instead of narrowing before std::min; after each
replacement->read(...) check for negative/error and on error set an error flag
and break/return without performing the zero-pad, and after each
isoFile->write(...) verify the write succeeded (handle short/written<requested
as an error). If an error occurs, ensure you stop the coroutine and report the
failure (e.g., via m_extractionProgress or an error return) rather than
continuing to pad; reference IO<File> replacement, CDRIsoFile isoFile,
replacement->read, isoFile->write, replaceSize, written and m_extractionProgress
to locate where to add these checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 45816883-82dc-4151-8087-8755ed00bc7b
📒 Files selected for processing (4)
src/cdrom/iso9660-reader.ccsrc/core/luaiso.ccsrc/gui/widgets/isobrowser.ccsrc/gui/widgets/isobrowser.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/gui/widgets/isobrowser.h
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/core/luaiso.cc (1)
129-143:⚠️ Potential issue | 🟠 MajorPrevent recursive directory cycles and drop zero-length extents in
collectAllEntries.
collectAllEntriesstill recurses on every directory without a visited guard, so a malformed ISO can cause unbounded recursion. Also,{lba, 0}extents should be skipped to keep gap aggregation stable. This is the root cause for potential hangs/stack overflow and downstream gap artifacts inreaderFindGaps(Line 175 onward).Proposed fix
+#include <unordered_set> @@ static void collectAllEntries(PCSX::ISO9660Reader* reader, const PCSX::ISO9660LowLevel::DirEntry& dir, - std::vector<std::pair<uint32_t, uint32_t>>& out) { + std::vector<std::pair<uint32_t, uint32_t>>& out, + std::unordered_set<uint32_t>& visitedDirs) { @@ uint32_t sectors = (size + sectorSize - 1) / sectorSize; - out.push_back({lba, sectors}); + if (sectors != 0) out.push_back({lba, sectors}); bool isDir = (entry.get<PCSX::ISO9660LowLevel::DirEntry_Flags>().value & 2) != 0; - if (isDir) collectAllEntries(reader, entry, out); + if (isDir && visitedDirs.insert(lba).second) { + collectAllEntries(reader, entry, out, visitedDirs); + } } } @@ - collectAllEntries(reader, reader->getRootDirEntry(), allFiles); + std::unordered_set<uint32_t> visitedDirs; + visitedDirs.insert(rootLBA); + collectAllEntries(reader, reader->getRootDirEntry(), allFiles, visitedDirs);Also applies to: 170-170
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/luaiso.cc` around lines 129 - 143, collectAllEntries currently recurses into directories without tracking visited directories and pushes extents with zero sectors; to fix, add a visited set (e.g., std::unordered_set<uint32_t> visitedLbas or pair<lba, size> if needed) keyed by each directory's LBA (use entry.get<PCSX::ISO9660LowLevel::DirEntry_LBA>() for the dir) and skip recursing if the dir LBA is already in the set, and before out.push_back({lba, sectors}) skip pushing when sectors == 0 so zero-length extents are omitted; update collectAllEntries signature or capture scope as needed to carry the visited set while still calling reader->listAllEntriesFrom and preserving existing LBA/sector calculation logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/core/luaiso.cc`:
- Around line 129-143: collectAllEntries currently recurses into directories
without tracking visited directories and pushes extents with zero sectors; to
fix, add a visited set (e.g., std::unordered_set<uint32_t> visitedLbas or
pair<lba, size> if needed) keyed by each directory's LBA (use
entry.get<PCSX::ISO9660LowLevel::DirEntry_LBA>() for the dir) and skip recursing
if the dir LBA is already in the set, and before out.push_back({lba, sectors})
skip pushing when sectors == 0 so zero-length extents are omitted; update
collectAllEntries signature or capture scope as needed to carry the visited set
while still calling reader->listAllEntriesFrom and preserving existing
LBA/sector calculation logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d9071aca-3ff8-4338-92d5-45636afd4d35
📒 Files selected for processing (1)
src/core/luaiso.cc
- Flat view trailing gap: bound by iso->getTD(0).toLBA() (actual loaded image end) instead of PVD_VolumeSpaceSize (logical volume length). - Extract/Replace now persist the sector mode alongside the selection and pass it into CDRIsoFile, matching Hex Edit; gaps/system use RAW, hidden files use their detected M1/M2_FORM1/M2_FORM2 mode. - Replace coroutine: aborts on I/O errors with a printed message instead of silently zero-padding over read/write failures. Also clamps replacement->size() without truncating through uint32_t. - Gap scan: unrecognized sector modes now accumulate into a gap entry (instead of silently dropping bytes) and the scan yields periodically so long runs don't freeze the UI. - readerListDir: the C++ side now strips "." / ".." sentinel entries so Lua callers never see them, regardless of how LuaJIT's ffi.string happens to encode the single-NUL filename. - collectAllEntries (Lua) and collectFlatEntries (GUI) now carry a visited-LBA set to guard against directory cycles in malformed ISOs. Zero-length extents are also skipped in the Lua side so they don't confuse gap aggregation. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
Member
Author
|
Third review round addressed in 3113760:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/gui/widgets/isobrowser.cc`:
- Around line 638-658: The extraction coroutine currently ignores I/O failure
states; update the lambda (the async block constructing IO<File> src = new
CDRIsoFile(...) and IO<File> out = new UvFile(...)) to check src->failed() and
the return value of out->write() (and out->failed()) on every operation: when
read() <= 0 or src->failed() occurs, mark extraction as failed and abort
(co_return) instead of continuing to progress to 1.0f; when calling
out->write(buffer, read) capture its return (bytesWritten) and if bytesWritten
!= read or out->failed() treat as a write failure, abort, and ensure the partial
file is handled (close/truncate or remove) and set self->m_extractionProgress to
an error state or not 1.0f so callers know it didn’t succeed. Ensure these
checks are applied inside the read/write loop and immediately after the loop
before setting m_extractionProgress = 1.0f.
- Around line 782-790: ReadFn currently returns an uninitialized byte if
inst.m_file->readAt fails or reads fewer bytes; initialize the local ImU8 b = 0
and check the return/bytes-read from inst.m_file->readAt in inst.m_editor.ReadFn
so you return 0 on error/short-read. Likewise, in inst.m_editor.Cache.BulkReadFn
check the result of inst.m_file->readAt and, if it returns an error or fewer
bytes than requested, explicitly zero-fill the remaining bytes in dest to avoid
exposing stale memory; keep the existing inst.m_editor.WriteFn as-is but ensure
read paths handle partial/failed I/O.
- Around line 60-132: drawFilesystemTree currently recurses into directories
without tracking visited directory LBAs, which can lead to infinite recursion on
malformed ISOs; add a visitedDirs set (same type/name used in flat view) and
check/insert the directory's LBA before recursing in drawFilesystemTree (use the
DirEntry_LBA value you already read into lba) to skip recursion when an LBA is
already visited, and ensure the root caller seeds/clears visitedDirs before the
initial draw so the guard applies for the whole tree traversal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 550ca676-7426-4e18-bf00-c0f3148872fe
📒 Files selected for processing (4)
src/core/isoffi.luasrc/core/luaiso.ccsrc/gui/widgets/isobrowser.ccsrc/gui/widgets/isobrowser.h
- drawFilesystemTree now carries a visited-LBA set and skips recursing into already-seen directories, guarding against infinite recursion on malformed ISOs with directory cycles. Matches the guard already in place on collectFlatEntries. - Extraction coroutine now checks src->failed() up front and verifies every read/write return, aborting with a printed error instead of silently marking progress as complete on disk-full or short-write. - Hex editor ReadFn defaults to 0 and returns 0 on short reads; the BulkReadFn zero-fills any tail bytes that readAt didn't cover so I/O errors don't expose stale buffer contents in the hex view. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/gui/widgets/isobrowser.cc (1)
139-162: Minor consistency nits withluaiso.cc.Two small drifts between
collectFlatEntrieshere andcollectAllEntriesinsrc/core/luaiso.cc(lines 143-164):
- Line 153 uses the bare literal
0x1000;luaiso.ccline 141 already definesXA_ATTR_FORM2 = 0x1000for the same bit. Promoting this to a shared header (e.g., alongside the other ISO9660 XA constants) would avoid the magic number appearing twice.- Line 155 pushes the
FlatEntryunconditionally, whereascollectAllEntriesnow skips zero-length extents (if (sectors != 0)). Zero-byte files will still show up in the flat view withsectors == 0. It's not actively buggy — the subsequent gap-insertion loop handlesend == lbafine — but it does surface phantom 0-sector rows that the gap scanner'stotalGapSectorsaccounting won't account for either.♻️ Sketch
- // 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}); + // 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 & XA_ATTR_FORM2) ? 2324 : 2048; + uint32_t sectors = (size + sectorSize - 1) / sectorSize; + if (sectors != 0 || isDir) { + m_flatEntries.push_back( + {fullPath, lba, size, sectors, isDir ? FlatEntry::Directory : FlatEntry::File, dirEntry}); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/widgets/isobrowser.cc` around lines 139 - 162, The collectFlatEntries function uses a magic literal and unconditionally pushes zero-sector entries; update it to use the shared XA_ATTR_FORM2 constant instead of 0x1000 (look for XA_ATTR_FORM2 where XA attributes are defined) and only append a FlatEntry when computed sectors != 0 (i.e., after computing xaAttribs, sectorSize and sectors, guard the m_flatEntries.push_back(...) with if (sectors != 0)). Also ensure the translation unit includes the header that defines XA_ATTR_FORM2 so the symbol resolves.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/gui/widgets/isobrowser.cc`:
- Around line 139-162: The collectFlatEntries function uses a magic literal and
unconditionally pushes zero-sector entries; update it to use the shared
XA_ATTR_FORM2 constant instead of 0x1000 (look for XA_ATTR_FORM2 where XA
attributes are defined) and only append a FlatEntry when computed sectors != 0
(i.e., after computing xaAttribs, sectorSize and sectors, guard the
m_flatEntries.push_back(...) with if (sectors != 0)). Also ensure the
translation unit includes the header that defines XA_ATTR_FORM2 so the symbol
resolves.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fbd71430-223e-454a-a103-6cda2a1ab486
📒 Files selected for processing (4)
src/core/isoffi.luasrc/core/luaiso.ccsrc/gui/widgets/isobrowser.ccsrc/gui/widgets/isobrowser.h
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.