Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 66 additions & 0 deletions systems/ps4.ixx
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,73 @@ public:
serial.insert(4, "-");
os << std::format(" serial: {}", serial) << std::endl;
}

std::string content_ids = getContentIds(root_directory, _PKG_FILE_NAMES);

if(content_ids.empty())
return;

os << std::format(" content ID(s): {}", content_ids) << std::endl;
}

protected:
std::string getContentIds(std::shared_ptr<iso9660::Entry> root_directory, std::span<const std::string> pkg_file_names) const
{
auto app_directory = root_directory->subEntry("app");
if(!app_directory)
return "";

auto app_directory_entries = app_directory->entries();
std::string content_ids;

for(auto &e : app_directory_entries)
{
if(!e->isDirectory())
continue;

for(const auto& pkg_file_name : pkg_file_names)
{
auto app_pkg_entry = e->subEntry(pkg_file_name);
if(!app_pkg_entry)
continue;

auto app_pkg_raw = app_pkg_entry->read();

if(app_pkg_raw.size() < _PKG_HEADER_SIZE)
continue;
Comment on lines +87 to +90

@coderabbitai coderabbitai Bot Jun 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Avoid full-file reads for PKG header parsing.

At Line 87, app_pkg_entry->read() loads the entire PKG even though this logic only needs header bytes up to _PKG_HEADER_SIZE (0x1000). For large PS4/PS5 PKGs, this can significantly increase redumper info latency and memory pressure. Prefer a bounded header read (or a header-only helper) before magic/content-ID checks.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@systems/ps4.ixx` around lines 87 - 90, The current implementation in the
app_pkg_entry->read() call loads the entire PKG file contents, but the
subsequent logic only needs the header bytes up to _PKG_HEADER_SIZE. Replace the
full-file read with a bounded read operation that retrieves only the header
bytes (0x1000 bytes as defined by _PKG_HEADER_SIZE) to reduce latency and memory
consumption for large PKG files. Ensure the bounded read still provides enough
data for the size validation check and any subsequent magic/content-ID checks
that depend on the header.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the read() opens the pointer to the data that then can be read on demand? Unsure if I am in the wrong or AI.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find iso9660 filesystem module files
fd -e ixx -e hh -e h -e cc -e cpp iso9660

# Search for the read() method implementation in iso9660
rg -n "read" --type-add 'ixx:*.ixx' --type ixx -C3

Length of output: 203090



@RobbedColek — good instinct to question it, but having looked at the actual implementation in filesystem/iso9660/iso9660_entry.ixx, read() is an eager, full-file load, not a lazy reader:

std::vector<uint8_t> read(bool form2 = false, bool *form_hint = nullptr)
{
    // allocates a buffer for the ENTIRE file upfront
    std::vector<uint8_t> sectors(sectorsSize() * _dataReader->sectorSize(form2));

    // reads ALL sectors into the buffer immediately
    uint32_t sectors_read = _dataReader->read(sectors.data(), sectorsLBA(), sectorsSize(), form2, form_hint);

    sectors.resize(size);
    return sectors; // returns the full contents
}

It pre-allocates sectorsSize() × sectorSize bytes (the whole file), calls through to _dataReader->read() (which hits the underlying file stream), and returns a fully populated std::vector<uint8_t>. There is no lazy/streaming interface here.

So the concern is valid: for a PS4/PS5 app PKG that can easily be several gigabytes, app_pkg_entry->read() will attempt to load the entire file into RAM just to check the first 0x40 + 36 = 76 bytes. A bounded read (if the underlying reader supports seeking + partial reads) or a dedicated header-reading helper would be significantly more efficient.


🧠 Learnings used
Learnt from: Didrole
Repo: superg/redumper PR: 356
File: systems/gc.ixx:53-59
Timestamp: 2026-02-26T22:08:07.395Z
Learning: In systems/gc.ixx and systems/wii.ixx (and other files under systems with .ixx extension), when BCA file validation fails (e.g., unexpected size), the function should return silently and not emit a warning. Reviewers should verify that BCA validation failures do not log warnings and consider adding tests to cover the silent failure behavior.

Learnt from: Didrole
Repo: superg/redumper PR: 356
File: systems/wii.ixx:54-60
Timestamp: 2026-02-26T22:08:15.577Z
Learning: In systems/wii.ixx and systems/gc.ixx, when a .bca file exists but its size does not match _WII_BCA_STRUCTURE_SIZE or _GC_BCA_STRUCTURE_SIZE, the code is expected to skip silently without emitting a warning or error. This behavior should be consistently maintained across similar modules. If this pattern could appear in other modules, prefer the glob pattern systems/**/*.ixx to cover all related files. Consider adding a brief inline comment near the relevant size-check to document the rationale, and add a targeted unit/integration test to verify there is no warning or error emitted for mismatched sizes.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RobbedColek looks like this is legit callout, those pkg files are huge.
It's a little bit inconvenient but I would ask instead of using directory read(), use data_reader directly and directory entry sectorsLBA() method to get LBA and only read what you need.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!


uint32_t app_pkg_magic =
(static_cast<uint32_t>(app_pkg_raw[_PKG_MAGIC_OFFSET + 0]) << 24) |
(static_cast<uint32_t>(app_pkg_raw[_PKG_MAGIC_OFFSET + 1]) << 16) |
(static_cast<uint32_t>(app_pkg_raw[_PKG_MAGIC_OFFSET + 2]) << 8) |
(static_cast<uint32_t>(app_pkg_raw[_PKG_MAGIC_OFFSET + 3]) << 0);
Comment on lines +92 to +96

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


if(app_pkg_magic != _PKG_MAGIC)
continue;

std::span<const uint8_t> app_pkg_content_id(app_pkg_raw.data() + _PKG_CONTENT_ID_OFFSET, _PKG_CONTENT_ID_SIZE);

std::string_view app_pkg_content_id_text(reinterpret_cast<const char*>(app_pkg_content_id.data()), _PKG_CONTENT_ID_SIZE);

if(!content_ids.empty())
content_ids += ", ";

content_ids += app_pkg_content_id_text;

break;
}
}

return content_ids;
}

private:
static constexpr uint32_t _PKG_HEADER_SIZE = 0x1000;
static constexpr uint32_t _PKG_MAGIC = 0x7F434E54;
static constexpr uint32_t _PKG_MAGIC_OFFSET = 0x00;
static constexpr uint32_t _PKG_CONTENT_ID_OFFSET = 0x40;
static constexpr uint32_t _PKG_CONTENT_ID_SIZE = 36;
Comment on lines +118 to +122

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd ask you to define a FileHeader structure, like here: https://www.psdevwiki.com/ps4/PKG_files and just use file_header.pkg_content_id in the above code.

In general I prefer the code to be clear to understand for somebody who wants to learn what is read and why is read.

static constexpr std::array<std::string, 3> _PKG_FILE_NAMES = {"app.pkg", "app_h.pkg", "app_0.pkg"};
};

}
12 changes: 11 additions & 1 deletion systems/ps5.ixx
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ export module systems.ps5;

import filesystem.iso9660;
import readers.data_reader;
import systems.ps4;
import utils.misc;
import utils.strings;

Expand All @@ -19,7 +20,7 @@ import utils.strings;
namespace gpsxre
{

export class SystemPS5 : public System
export class SystemPS5 : public SystemPS4
{
public:
std::string getName() override
Expand Down Expand Up @@ -48,9 +49,18 @@ public:

if(auto it = param_json.find("masterDataId"); it != param_json.end())
os << std::format(" serial: {}", it->second.insert(4, "-")) << std::endl;

std::string content_ids = getContentIds(root_directory, _PKG_FILE_NAMES);

if(content_ids.empty())
return;

os << std::format(" content ID(s): {}", content_ids) << std::endl;
}

private:
static constexpr std::array<std::string, 1> _PKG_FILE_NAMES = {"app_sc.pkg"};

std::map<std::string, std::string> loadJSON(std::shared_ptr<iso9660::Entry> json_entry) const
{
std::map<std::string, std::string> json;
Expand Down