Skip to content

Print PS3 firmware version#385

Merged
superg merged 5 commits into
superg:mainfrom
Deterous:ps3fw
Jun 19, 2026
Merged

Print PS3 firmware version#385
superg merged 5 commits into
superg:mainfrom
Deterous:ps3fw

Conversation

@Deterous

@Deterous Deterous commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Prints PS3 Firmware version as part of redumper info
e.g. firmware version: 2.80

Summary by CodeRabbit

  • New Features
    • PS3 system information now extracts and displays the firmware version from the console update file inside the disc image.
    • Firmware details are shown alongside existing version and serial information to improve diagnostics and visibility; the extractor fails gracefully when an update file or valid version cannot be read.

@coderabbitai

coderabbitai Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

SystemPS3::printInfo now attempts to read PS3_UPDATE/PS3UPDAT.PUP from the ISO and, when a firmware string is found, prints "firmware version: ...". New private PUP parsing adds constants, a PUPHeader layout, sector buffering support, and pupVersion(...) that reads a sector, validates headers, locates the version record, and extracts 4 bytes.

Changes

PS3 Firmware Version Extraction

Layer / File(s) Summary
Sector buffering dependency
systems/ps3.ixx
Added <vector> include for buffering ISO sectors used by the firmware parser.
PUP header and constants
systems/ps3.ixx
Introduced private constants and PUPHeader/PUPFile struct layout used by the parser and print integration.
Firmware version parser
systems/ps3.ixx
Added pupVersion(...) that reads the PUP sector via DataReader, validates the PUP header, scans records to compute the firmware version offset, and extracts a 4-byte firmware version with bounds checks.
Firmware version display in system info
systems/ps3.ixx
Extended SystemPS3::printInfo to call pupVersion(...) for PS3_UPDATE/PS3UPDAT.PUP and print firmware version: ... when a non-empty value is returned.
sequenceDiagram
  participant SystemPS3
  participant PUPParser
  participant DataReader
  SystemPS3->>PUPParser: request pupVersion("PS3_UPDATE/PS3UPDAT.PUP")
  PUPParser->>DataReader: read sector at file.sectorsLBA()
  DataReader-->>PUPParser: sector buffer
  PUPParser->>PUPParser: validate header, iterate records, compute offset
  PUPParser-->>SystemPS3: firmware version string or empty
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 In sectors hidden, bytes convene,
A tiny PUP with secrets seen.
I hop through headers, offsets true,
I fetch four chars that speak the view.
Firmware unveiled — a rabbit's cue!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Print PS3 firmware version' accurately and concisely summarizes the main change: adding firmware version output to PS3 info printing.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

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.

Actionable comments posted: 1

🤖 Prompt for all review comments with 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.

Inline comments:
In `@systems/ps3.ixx`:
- Around line 168-185: Validate the 4-byte firmware slice against the PUP entry
length and read across sector boundary if needed: compute the absolute offset
(version_offset) relative to the start of the pup entry (use
pup_file->length()/entry length and pup_file->sectorsLBA()), bail if
version_offset + 4 > pup_file->length(); otherwise determine which sector(s)
contain those 4 bytes and ensure you fill sector_buffer(s) for both sectors when
the 4-byte field straddles a sector boundary (use data_reader->read on
pup_file->sectorsLBA() + sector index and data_reader->sectorSize()), then
extract the 4 bytes from the assembled buffer into firmware_version; reference
symbols: version_offset, pup_file->sectorsLBA(), pup_file->length(),
data_reader->sectorSize(), DataReader::read, sector_buffer, firmware_version.
🪄 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: defaults

Review profile: CHILL

Plan: Pro

Run ID: 41fa4888-0896-474e-bacc-cd262d575e03

📥 Commits

Reviewing files that changed from the base of the PR and between d4fcfe1 and 0643be2.

📒 Files selected for processing (1)
  • systems/ps3.ixx

Comment thread systems/ps3.ixx Outdated
Comment thread systems/ps3.ixx
if(!pup_file)
return firmware_version;

std::vector<uint8_t> sector_buffer(data_reader->sectorSize());

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.

You unconditionally jump to 0x3E location. PUP internally has a header that defines records count. You need to iterate records and find 0x100 record type that would be version (I once wrote PUP extractor / packer), jump to it's offset and read the resource.

{
private:
	static const u32 m_BUFFER_SIZE = 0x400 * 0x400; // 1Mb
	static const int m_DIGEST_SIZE = 20;
	static const int m_HMAC_KEY_SIZE = 0x40;
	static const u8 m_HMAC_KEY[m_HMAC_KEY_SIZE];
	static const char *m_MAGIC;
	static const string m_CONFIG_FILENAME;
	static const string m_HEX_PREFIX;
	
	struct Header
	{
		u8 magic[8];
		u64 version;
		u64 build;
		u64 records_count;
		u64 header_size;
		u64 data_size;
	};

	struct FileRecord
	{
		struct File
		{
			u64 type;
			u64 offset;
			u64 size;
			u64 padding;
		} file;
		
		struct Signature
		{
			u64 index;
			u8 hash[m_DIGEST_SIZE];
			u32 padding;
		} signature;
	};

	struct GlobalSignature
	{
		u8 hash[m_DIGEST_SIZE];
		u8 padding[12];
	};
	
	struct FileIdNameRecord
	{
		u64 id;
		string name;
	};
	
	struct Context
	{
		Header header;
		vector<FileRecord> records;
		GlobalSignature signature;
		map<u64, string> m_FilenameMap;
	};
	
	static const FileIdNameRecord m_ID_NAME_TABLE[];
	
private:
	streampos GetFileSize(ifstream &);
	string GetFilenameByType(u64, u64);
	bool LoadConfigFromFirmware(Context &ctx, string);
	bool LoadConfig(Context &ctx, string);
	bool SaveConfig(Context &ctx, string);
	void PrintConfig(Context &ctx);
	
public:
	bool ExtractFirmware(string, string);
	bool CreateFirmware(string, string);
};```

```const char *PS3PupProcessor::m_MAGIC = "SCEUF\0\0\0";
const string PS3PupProcessor::m_CONFIG_FILENAME("config.bin");
const string PS3PupProcessor::m_HEX_PREFIX("0x");

const PS3PupProcessor::FileIdNameRecord PS3PupProcessor::m_ID_NAME_TABLE[] =
{
	{0x100, string("version.txt")},
	{0x101, string("dots.txt")},
	{0x102, string("promo.txt")},
	{0x103, string("inc.txt")},
	{0x104, string("version_inc.txt")},
	{0x200, string("updater.self")},
	{0x201, string("vsh.tar")},
	{0x202, string("msg.xml")},
	{0x203, string("updater_inc.self")},
	{0x300, string("update.tar")}
};


streampos PS3PupProcessor::GetFileSize(ifstream &arg_if)
{
	streampos last_pos = arg_if.tellg();
	arg_if.seekg(0, ios::end);
	streampos result = arg_if.tellg();
	arg_if.seekg(last_pos);
	
	return result;
}


string PS3PupProcessor::GetFilenameByType(u64 a_id, u64 a_build)
{
	string result;
	
	u64 id = a_id;
	// WORKAROUND: starting from 2.40, dots.txt and msg.xml were swapped
	if(a_build >= 0x427f)
	{
		if(id == 0x101)
			id = 0x202;
		else if(id == 0x202)
			id = 0x101;
	}
	
	bool found = false;
	for(unsigned int i = 0; i < sizeof(m_ID_NAME_TABLE) / sizeof(m_ID_NAME_TABLE[0]); ++i)
	{
		if(m_ID_NAME_TABLE[i].id == id)
		{
			result = m_ID_NAME_TABLE[i].name;
			found = true;
			break;
		}
	}
	
	if(!found)
	{
		char buffer[0x100];
		sprintf(buffer, "unknown_%08X.bin", (int)a_id);
		result = string(buffer);
	}
	
	return result;
}


bool PS3PupProcessor::LoadConfigFromFirmware(Context &ctx, string a_input_fn)
{
	bool result = false;
	
	ifstream iF;
	iF.open(a_input_fn.c_str(), ios::binary);
	if(!iF.is_open())
	{
		cout << "error: unable to open file " << a_input_fn << endl;
		return result;
	}

	// read header
	iF.read((char *)&ctx.header, sizeof(ctx.header));
	
	// check magic
	if(memcmp(ctx.header.magic, m_MAGIC, sizeof(ctx.header.magic)))
	{
		cout << "error: " << a_input_fn << " isn't firmware PUP-file" << endl;
		return result;
	}
	
	// TRICK: we can't fix ctx.header.records_count, because we need to check hash before
	u64 records_count = ctx.header.records_count;
	BE2SYS(records_count);
	ctx.records.resize(records_count);
	
	// read file records
	for(u64 i = 0; i < records_count; ++i)
		iF.read((char *)&ctx.records[i].file, sizeof(ctx.records[i].file));
	
	// read signatures
	for(u64 i = 0; i < records_count; ++i)
		iF.read((char *)&ctx.records[i].signature, sizeof(ctx.records[i].signature));
	
	// read and check global signature
	iF.read((char *)&ctx.signature, sizeof(ctx.signature));
	u8 digest[m_DIGEST_SIZE];
	Sha1HmacCtx sha1_hmac_ctx;
	sha1_hmac_init(sha1_hmac_ctx, m_HMAC_KEY);
	sha1_hmac_update(sha1_hmac_ctx, (u8 *)&ctx.header, sizeof(ctx.header));
	for(u64 i = 0; i < records_count; ++i)
		sha1_hmac_update(sha1_hmac_ctx, (u8 *)&ctx.records[i].file, sizeof(ctx.records[i].file));
	for(u64 i = 0; i < records_count; ++i)
		sha1_hmac_update(sha1_hmac_ctx, (u8 *)&ctx.records[i].signature, sizeof(ctx.records[i].signature));
	sha1_hmac_final(sha1_hmac_ctx, digest);
	
	bool digest_diff = memcmp(ctx.signature.hash, digest, m_DIGEST_SIZE);
	
	cout << "global signature "
		<< (digest_diff ? "wrong" : "correct") << endl << endl;
	
	// convert endianness to system
	BE2SYS(ctx.header.version);
	BE2SYS(ctx.header.build);
	BE2SYS(ctx.header.records_count);
	BE2SYS(ctx.header.header_size);
	BE2SYS(ctx.header.data_size);
	for(u64 i = 0; i < records_count; ++i)
	{
		BE2SYS(ctx.records[i].file.type);
		BE2SYS(ctx.records[i].file.offset);
		BE2SYS(ctx.records[i].file.size);
	}
	for(u64 i = 0; i < records_count; ++i)
		BE2SYS(ctx.records[i].signature.index);

	iF.close();
	
	result = true;
	return result;
}


bool PS3PupProcessor::LoadConfig(Context &ctx, string a_input_fn)
{
	bool result = false;
	
	ifstream iF(a_input_fn.c_str(), ios::binary);
	if(!iF.is_open())
	{
		cout << "error: unable to open file " << a_input_fn << endl;
		return result;
	}
	
	memcpy((char *)ctx.header.magic, m_MAGIC, sizeof(ctx.header.magic));
	iF.read((char *)&ctx.header.version, sizeof(ctx.header.version));
	iF.read((char *)&ctx.header.build, sizeof(ctx.header.build));
	iF.read((char *)&ctx.header.records_count, sizeof(ctx.header.records_count));
	ctx.records.resize(ctx.header.records_count);
	for(u64 i = 0; i < ctx.header.records_count; ++i)
	{
		iF.read((char *)&ctx.records[i].file.type, sizeof(ctx.records[i].file.type));
		ctx.records[i].signature.index = i;
	}
	
	ctx.header.records_count = ctx.records.size();
	ctx.header.header_size = sizeof(ctx.header) +
		sizeof(FileRecord) * ctx.header.records_count + sizeof(ctx.signature);
	
	result = true;
	return result;
}


bool PS3PupProcessor::SaveConfig(Context &ctx, string a_output_fn)
{
	bool result = false;
	
	ofstream oF(a_output_fn.c_str(), ios::binary);
	if(!oF.is_open())
	{
		cout << "error: unable to create file " << a_output_fn << endl;
		return result;
	}
	
	oF.write((char *)&ctx.header.version, sizeof(ctx.header.version));
	oF.write((char *)&ctx.header.build, sizeof(ctx.header.build));
	oF.write((char *)&ctx.header.records_count, sizeof(ctx.header.records_count));
	for(u64 i = 0; i < ctx.header.records_count; ++i)
		oF.write((char *)&ctx.records[i].file.type, sizeof(ctx.records[i].file.type));
	
	result = true;
	return result;
}


void PS3PupProcessor::PrintConfig(Context &ctx)
{
	cout << setfill('0');

	// output file info
	cout << "GENERAL: " << endl;
	cout << "version: " << m_HEX_PREFIX << hex << setw(2)
		<< ctx.header.version << endl;
	cout << "build: " << m_HEX_PREFIX << hex << ctx.header.build << endl;
	cout << "signature: " << m_HEX_PREFIX;
		for(int j = 0; j < m_DIGEST_SIZE; ++j)
			cout << hex << setw(2) << (int)ctx.signature.hash[j];
	cout << endl;
	cout << "records count: " << dec << ctx.header.records_count << endl << endl;

	// records
	for(u64 i = 0; i < ctx.header.records_count; ++i)
	{
		cout << dec << "RECORD " << i + 1 << ": " << endl;
		cout << "type: " << m_HEX_PREFIX << hex << ctx.records[i].file.type
			<< " [" << GetFilenameByType(ctx.records[i].file.type, ctx.header.build) << "]" << endl;
		cout << "size: " << dec << ctx.records[i].file.size << endl;

		cout << "signature: " << m_HEX_PREFIX;
		for(int j = 0; j < m_DIGEST_SIZE; ++j)
			cout << hex << setw(2) << (int)ctx.records[i].signature.hash[j];
		cout << endl;
		cout << endl;
	}

	cout << endl;
}


bool PS3PupProcessor::ExtractFirmware(string a_input_fn, string a_output_path)
{
	bool result = false;
	Context ctx;

	if(!LoadConfigFromFirmware(ctx, a_input_fn))
		return result;
	
	ifstream iF;
	iF.open(a_input_fn.c_str(), ios::binary);
	if(!iF.is_open())
	{
		cout << "error: unable to open file " << a_input_fn << endl;
		return result;
	}

	// data extraction
	char *buffer = new char[m_BUFFER_SIZE];
	for(u64 i = 0; i < ctx.header.records_count; ++i)
	{
		cout << setfill('0');

		// show info about file
		string output_filename = GetFilenameByType(ctx.records[i].file.type, ctx.header.build);
		cout << "extracting file " << output_filename << endl;
		cout << "type: " << m_HEX_PREFIX << hex
			<< ctx.records[i].file.type << endl;
		cout << "size: " << dec << ctx.records[i].file.size << endl;

		streampos size = (streampos)ctx.records[i].file.size;
		streampos left_bytes = size;
		iF.seekg(ctx.records[i].file.offset);

		filesystem::create_directory(a_output_path);
		filesystem::path output_path(filesystem::path(a_output_path)
			/ filesystem::path(output_filename));
		
		ofstream oF;
		oF.open(output_path.string().c_str(), ios::binary);
		if(!oF.is_open())
		{
			cout << "error: unable to create file " << a_input_fn << endl;
			delete [] buffer;
			return result;
		}
		
		u8 digest[m_DIGEST_SIZE];
		Sha1HmacCtx sha1_hmac_ctx;
		sha1_hmac_init(sha1_hmac_ctx, m_HMAC_KEY);
		
		cout << setfill(' ');
		while(left_bytes)
		{
			int to_read = left_bytes > m_BUFFER_SIZE ?
				m_BUFFER_SIZE : (int)left_bytes;

			left_bytes -= to_read;
			iF.read(buffer, to_read);
			oF.write(buffer, to_read);
			
			sha1_hmac_update(sha1_hmac_ctx, (u8 *)buffer, to_read);
			
			int percentage = (1 - left_bytes / (float)size) * 100;
			cout << '\r';
			cout << "progress: ";
			cout.setf(ios::right, ios::basefield);
			cout.width(3);
			cout << percentage << '%' << flush;
		}
		
		sha1_hmac_final(sha1_hmac_ctx, digest);
		
		bool digest_diff = memcmp(ctx.records[i].signature.hash, digest, m_DIGEST_SIZE);
		
		cout << endl << "done, signature "
			<< (digest_diff ? "wrong" : "correct") << endl << endl;
		
		oF.close();
	}
	
	delete [] buffer;
	
	iF.close();
	SaveConfig(ctx, (filesystem::path(a_output_path) / filesystem::path(m_CONFIG_FILENAME)).string());
	
	result = true;
	return result;
}


bool PS3PupProcessor::CreateFirmware(string a_output_fn, string a_input_path)
{
	bool result = false;
	Context ctx;
	
	if(!LoadConfig(ctx, (filesystem::path(a_input_path) / filesystem::path(m_CONFIG_FILENAME)).string()))
		return result;
	
	ofstream oF;
	oF.open(a_output_fn.c_str(), ios::binary);
	if(!oF.is_open())
	{
		cout << "error: unable to create file " << a_output_fn << endl;
		return result;
	}

	ctx.header.data_size = 0;	
	oF.seekp(ctx.header.header_size);
	
	char *buffer = new char[m_BUFFER_SIZE];
	for(u64 i = 0; i < ctx.header.records_count; ++i)
	{
		cout << setfill('0');
		
		string input_filename = GetFilenameByType(ctx.records[i].file.type, ctx.header.build);
		
		filesystem::path input_path(filesystem::path(a_input_path)
			/ filesystem::path(input_filename));
		
		ifstream iF;
		iF.open(input_path.string().c_str(), ios::binary);
		if(!iF.is_open())
		{
			cout << "error: unable to open file " << input_filename << endl;
			delete [] buffer;
			return result;
		}
		
		ctx.records[i].file.offset = oF.tellp();
		ctx.records[i].file.size = GetFileSize(iF);
		ctx.header.data_size += ctx.records[i].file.size;

		cout << "writing file "<< input_filename << endl;
		cout << "type: " << m_HEX_PREFIX << hex
			<< ctx.records[i].file.type << endl;
		cout << "size: " << dec << ctx.records[i].file.size << endl;
		
		streampos size = (streampos)ctx.records[i].file.size;
		streampos left_bytes = size;
		
		Sha1HmacCtx sha1_hmac_ctx;
		sha1_hmac_init(sha1_hmac_ctx, m_HMAC_KEY);
		
		cout << setfill(' ');
		while(left_bytes)
		{
			int to_write = left_bytes > m_BUFFER_SIZE ?
				m_BUFFER_SIZE : (int)left_bytes;
			
			left_bytes -= to_write;
			iF.read(buffer, to_write);
			oF.write(buffer, to_write);

			sha1_hmac_update(sha1_hmac_ctx, (u8 *)buffer, to_write);
			
			int percentage = (1 - left_bytes / (float)size) * 100;
			cout << '\r';
			cout << "progress: ";
			cout.setf(ios::right, ios::basefield);
			cout.width(3);
			cout << percentage << '%' << flush;
		}
		
		sha1_hmac_final(sha1_hmac_ctx, ctx.records[i].signature.hash);
		
		cout << endl << "done" << endl << endl;
		
		iF.close();
	}
	
	delete [] buffer;
	
	// TRICK: preserve records_count, as it would be byteswapped to big endian
	u64 records_count = ctx.header.records_count;
	
	// clear paddings
	memset((void *)ctx.signature.padding, 0, sizeof(ctx.signature.padding));
	for(u64 i = 0; i < ctx.header.records_count; ++i)
		ctx.records[i].file.padding = 0;
	
	// convert header data back to big endian
	SYS2BE(ctx.header.version);
	SYS2BE(ctx.header.build);
	SYS2BE(ctx.header.records_count);
	SYS2BE(ctx.header.header_size);
	SYS2BE(ctx.header.data_size);
	for(u64 i = 0; i < records_count; ++i)
	{
		SYS2BE(ctx.records[i].file.type);
		SYS2BE(ctx.records[i].file.offset);
		SYS2BE(ctx.records[i].file.size);
	}
	for(u64 i = 0; i < records_count; ++i)
		SYS2BE(ctx.records[i].signature.index);
	
	// calculate global hash
	Sha1HmacCtx sha1_hmac_ctx;
	sha1_hmac_init(sha1_hmac_ctx, m_HMAC_KEY);
	sha1_hmac_update(sha1_hmac_ctx, (u8 *)&ctx.header, sizeof(ctx.header));
	for(u64 i = 0; i < records_count; ++i)
		sha1_hmac_update(sha1_hmac_ctx, (u8 *)&ctx.records[i].file, sizeof(ctx.records[i].file));
	for(u64 i = 0; i < records_count; ++i)
		sha1_hmac_update(sha1_hmac_ctx, (u8 *)&ctx.records[i].signature, sizeof(ctx.records[i].signature));
	sha1_hmac_final(sha1_hmac_ctx, ctx.signature.hash);
	
	// write header
	oF.seekp(0, ios::beg);
	oF.write((char *)&ctx.header, sizeof(ctx.header));
	for(u64 i = 0; i < records_count; ++i)
		oF.write((char *)&ctx.records[i].file, sizeof(ctx.records[i].file));
	for(u64 i = 0; i < records_count; ++i)
		oF.write((char *)&ctx.records[i].signature, sizeof(ctx.records[i].signature));
	oF.write((char *)&ctx.signature, sizeof(ctx.signature));
	
	oF.close();
	
	result = true;
	return result;
}```

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done. tried to implement it correctly but with as little complexity as possible

Comment thread systems/ps3.ixx Outdated
for(uint64_t i = 0; i < records_count; ++i)
{
// read next sector if needed
if(cur + 4 * sizeof(uint64_t) >= data_reader->sectorSize())

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.

This on-demand reading complicates code flow a lot.

Here's how flow should be:

  1. you read one sector, it has PUPHeader where you get records_count.
  2. you precompute how many sectors you need to read in order to get all records in one continous array: auto byte_size = sizeof(PUPHeader) + header->records_count * sizeof(FileRecord);
  3. you read N sectors that have all records (it's likely 1 or 2 sectors, I don't remember but still it's a safe thing to do to read)
  4. you iterate over records, find the one you need, calculate it's sector/sectors, read them and extract the value

Another thing, use structured data. Things like *(uint64_t *)&sector_buffer[cur + sizeof(uint64_t)] and cur += 4 * sizeof(uint64_t); do not tell anything at all, nobody will learn from this code and nothing is clear, it's almost like assembly. Structures help with understanding what code really does, use FileRecord, it's above in the copy pasted code that I provided.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For all known PUP files on PS3 discs, you only need to read the 1st sector, and version.txt is always in the same location (this is how MPF has always done it). Since you want it done properly, it has lead to this complexity

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.

In this case scrap dynamic sector reading, just throw exception if record spans beyond sector boundary and use structure.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

alright, it now reads the file struct as an object and returns if it ever needs more than 1 sector

@coderabbitai coderabbitai Bot left a comment

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.

Actionable comments posted: 1

🤖 Prompt for all review comments with 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.

Inline comments:
In `@systems/ps3.ixx`:
- Around line 208-211: The current slice check can overflow because
version_offset + 4 may wrap; compute version_offset = endian_swap(file->offset)
then perform an overflow-safe bounds check before slicing by ensuring
version_offset is not greater than sector_buffer.size() and that
sector_buffer.size() - version_offset >= 4 (or equivalently version_offset <=
sector_buffer.size() - 4) before using sector_buffer.data() + version_offset;
treat version_offset as a size_t (or cast) for the comparison and return "" on
failure as before.
🪄 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: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2ebb5a75-ee74-410f-a8eb-8e47b2f865db

📥 Commits

Reviewing files that changed from the base of the PR and between d429f74 and ee8cdc4.

📒 Files selected for processing (1)
  • systems/ps3.ixx

Comment thread systems/ps3.ixx Outdated

@coderabbitai coderabbitai Bot left a comment

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.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
systems/ps3.ixx (1)

192-211: ⚠️ Potential issue | 🟠 Major

Avoid UB when decoding PUPHeader/PUPFile from sector_buffer.
systems/ps3.ixx casts sector_buffer.data() to PUPHeader*/PUPFile* (lines 192 and 204), which can violate alignment and object-lifetime rules (UB, crash on strict-alignment platforms). Decode by std::memcpy into local structs before endian conversion.

💡 Minimal safe parsing change
-        auto header = (PUPHeader *)sector_buffer.data();
+        if(sector_buffer.size() < sizeof(PUPHeader))
+            return "";
+        PUPHeader header {};
+        std::memcpy(&header, sector_buffer.data(), sizeof(header));

-        if(memcmp(header->magic, _PUP_MAGIC.data(), _PUP_MAGIC.size()))
+        if(memcmp(header.magic, _PUP_MAGIC.data(), _PUP_MAGIC.size()))
             return "";

         uint32_t cur = sizeof(PUPHeader);
-        uint64_t records_count = endian_swap(header->records_count);
+        uint64_t records_count = endian_swap(header.records_count);
         for(uint64_t i = 0; i < records_count; ++i)
         {
             if(cur + sizeof(PUPFile) > sector_buffer.size())
                 return "";

-            auto file = (PUPFile *)(sector_buffer.data() + cur);
+            PUPFile file {};
+            std::memcpy(&file, sector_buffer.data() + cur, sizeof(file));
             cur += sizeof(PUPFile);
-            if(endian_swap(file->type) == 0x100)
+            if(endian_swap(file.type) == _VERSION_ID)
             {
-                uint64_t version_offset = endian_swap(file->offset);
-                if(version_offset > sector_buffer.size() - _VERSION_LENGTH)
+                uint64_t version_offset = endian_swap(file.offset);
+                if(version_offset > sector_buffer.size() || _VERSION_LENGTH > sector_buffer.size() - static_cast<size_t>(version_offset))
                     return "";
                 return std::string((char *)(sector_buffer.data() + version_offset), _VERSION_LENGTH);
             }
🤖 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/ps3.ixx` around lines 192 - 211, The code currently casts
sector_buffer.data() to PUPHeader*/PUPFile* which can cause UB; instead, copy
bytes into local POD instances with std::memcpy before inspecting fields: read a
local PUPHeader via memcpy from sector_buffer.data() then validate
magic/_PUP_MAGIC and records_count (use endian_swap on the local header), and
inside the loop memcpy into a local PUPFile from sector_buffer.data() + cur
before calling endian_swap on file->type/offset and using _VERSION_LENGTH; keep
the existing bounds checks (sector_buffer.size()) and return behavior but
operate on the memcpy'd local structs to avoid alignment/object-lifetime issues.
🤖 Prompt for all review comments with 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.

Outside diff comments:
In `@systems/ps3.ixx`:
- Around line 192-211: The code currently casts sector_buffer.data() to
PUPHeader*/PUPFile* which can cause UB; instead, copy bytes into local POD
instances with std::memcpy before inspecting fields: read a local PUPHeader via
memcpy from sector_buffer.data() then validate magic/_PUP_MAGIC and
records_count (use endian_swap on the local header), and inside the loop memcpy
into a local PUPFile from sector_buffer.data() + cur before calling endian_swap
on file->type/offset and using _VERSION_LENGTH; keep the existing bounds checks
(sector_buffer.size()) and return behavior but operate on the memcpy'd local
structs to avoid alignment/object-lifetime issues.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e693626c-d59f-4801-af6f-a61e0ac2813f

📥 Commits

Reviewing files that changed from the base of the PR and between ee8cdc4 and 7ab9435.

📒 Files selected for processing (1)
  • systems/ps3.ixx

@superg superg merged commit 6213d3c into superg:main Jun 19, 2026
11 checks passed
@Deterous Deterous deleted the ps3fw branch June 25, 2026 06:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants