Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add hashing support for PSX cheevos (bin/cue, chd, or real CD) #9405

Merged
merged 1 commit into from Sep 11, 2019

Conversation

Jamiras
Copy link
Contributor

@Jamiras Jamiras commented Aug 31, 2019

Guidelines

  1. Rebase before opening a pull request
  2. If you are sending several unrelated fixes or features, use a branch and a separate pull request for each
  3. If possible try squashing everything in a single commit. This is particularly beneficial in the case of feature merges since it allows easy bisecting when a problem arises

Description

retroachievements.org hashing algorithm implementation for Playstation 1 games

  • Logic is as follows: open the SYSTEM.CNF file on the disc and identify the executable specified by the BOOT= line. Then open and hash that file to generate the unique identifier for the game.
    • There are some games that use the same engine, so the filename is also included in the hash
    • This provides a unique identifier for each PSX disc, and multiple discs can be associated to a single game on the server.
    • Hashing the primary executable is a compromise between hashing the entire disc and using only the serial number. It ensures that the executable hasn't been modified, but does not guarantee that the supplemental data files have not been modified. For all of the discs that I tried, no executable exceeded 1.5MB.

Tested with bin/cue, chd, and real CD on Windows, and bin/cue and chd on Linux. The real CD was not recognized on Linux - I believe this to be a limitation of the VirtualBox passthrough interface.

Related Issues

n/a

Related Pull Requests

#9400 - alternate approach that tried to integrate the CD file system handling into the intfstream and filestream interfaces

This implementation is completely self-contained. It leverages the intfstream for reading sectors and does the directory navigation and data extraction as a layer between the intfsteam and the hashing code. It's written in such a way that it should be easy to port into the common code at some later point if desired.

Reviewers

@bparker06 @twinaphex

@ghost
Copy link

ghost commented Aug 31, 2019

I don't think an iso9660 parser should be confined to cheevos, we should have it available in a general form inside libretro-common.

@Jamiras
Copy link
Contributor Author

Jamiras commented Aug 31, 2019

I thought you were concerned about having it in libretro-common due to the incompleteness of the implementation - no iso support, additional sector sizes, dvd formats.

Should I move cdfs.c/h to cdrom, file, streams, media, vfs or somewhere else?

@ghost
Copy link

ghost commented Aug 31, 2019

The comments about sector sizes and such IMO are still valid even in the context of cheevos as it's still looking at the same content either way. For example if support were added for the Play! core, then we would need to be able to support 2048-byte sector DVD images. Or even for older systems, there may be dumps with other sector sizes than what is supported right now... I just didn't want someone to have to rewrite half of this to be able to make it more robust later on. But if the intent is to push this through sooner rather than later, even though I think it will be harder to deal with later, I guess I've said my peace and it's just up to @twinaphex now.

Anyways, probably "formats" would be a good place to put it in libretro-common since libchdr is already there.

@Jamiras
Copy link
Contributor Author

Jamiras commented Sep 1, 2019

  • Moved cdfs.c into libretro-common/formats/cdfs and cdfs.h into libretro-common/include/formats
  • Added cdfs_open_track method for opening a track by index.
  • Added cdfs_open_raw_track for opening bin and iso files.
  • Added cdfs_tell, cdfs_seek and cdfs_get_size helper methods.

I was able to successfully generate a hash for an iso created from a bin/cue (with 2048 byte sectors), but had to do so using custom code as the core doesn't support iso files.

I also used custom code to test opening a bin and a real CD track using the cdfs_open_raw_track function. Note that opening a real CD track requires opening the CD first as the vfs_cdrom_toc is only populated at that point.

@i30817
Copy link
Contributor

i30817 commented Sep 6, 2019

To be slightly alarmist, is this going to be used for normal cd ID in the future too? I mean this is fine for this usecase (sort of), but imagine the paragraphs below apply as if to a general scheme for ID.

Meaning should i modify my tool that produces hack checksums for libretro-database to be able to open cds of multiple platforms to search for 'executables' to hash? That's a bit farfetched in its implications and i hope not, because cd handling on multiple consoles is absolute nightmare and there is basically no 'ready to roll' library that handles all the edges from the CD consoles.

And although the executable is a 'ok' target for hack ID uniqueness it's by no means infallible because some games might have multiple executables (say a intro one) and upstreams checks the wrong one, or keep the important code elsewhere in a library dll, etc. Really the only way to be sure is all data checksum sadly.

@Jamiras
Copy link
Contributor Author

Jamiras commented Sep 6, 2019

This is specifically for RetroAchievements identification of the disc, and has no intended impact on any RetroArch specific identification logic.

By hashing the primary executable, we minimize the chance of the player user a modified copy of the game to gain an advantage. We acknowledge that the methodology isn't foolproof. While we have yet to encounter a game that has multiple executables, we know that this method does not prevent modification of the data files, and a dedicated cheater could modify a map file or enemy AI scripts or something similar. As mentioned above, this solution is a compromise between using the disc serial number and hashing the entire disc.

@i30817
Copy link
Contributor

i30817 commented Sep 7, 2019

Ok, i was being too rash here. I forgot/didn't understand that this application of the concept doesn't need a database of IDs, just agreement between participants the hash is the same! Thus the 'replacement' nonsense idea.

@inactive123
Copy link
Contributor

I hope everybody will be at ease with this being merged, and I believe I did request followup commentary on it. In case anybody still disagrees with the implementation here, contact me.

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.

None yet

3 participants