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

GCMemcard: Fix out of bounds access in F-Zero GX checksum calculation. #7765

Open
wants to merge 1 commit into
base: master
from

Conversation

4 participants
@AdmiralCurtiss
Copy link
Contributor

AdmiralCurtiss commented Jan 31, 2019

Pretty self-explanatory. block was incremented one loop too late; slight rewrite so it's clearer what it's doing.

This probably worked regardless because the data blocks are lying right next to eachother in memory, but Visual Studio's Debug build rightly complained about this.

@BhaaLseN
Copy link
Member

BhaaLseN left a comment

Looks a lot cleaner and more straight-forward like this!

@AdmiralCurtiss

This comment has been minimized.

Copy link
Contributor Author

AdmiralCurtiss commented Jan 31, 2019

Noticed another small thing, this never checked if the file actually had four data blocks, so if you constructed yourself a custom file with the correct filename but only one block you could trash 0x6000 bytes of memory. Unlikely, but hey.

@BhaaLseN

This comment has been minimized.

Copy link
Member

BhaaLseN commented Jan 31, 2019

That only checks for 4 blocks, but not really for the block size. Still a good addition tho.

@AdmiralCurtiss

This comment has been minimized.

Copy link
Contributor Author

AdmiralCurtiss commented Jan 31, 2019

Block size is fixed at 0x2000 bytes at a hardware level (and implemented as a fixed size array), so no real need to check that unless we expect memory card block sizes to change somehow.

@AdmiralCurtiss AdmiralCurtiss force-pushed the AdmiralCurtiss:fzero-save-file-out-of-bounds branch from 5d7e0ff to fdd19c1 Feb 1, 2019

@Tilka

This comment has been minimized.

Copy link
Member

Tilka commented Feb 2, 2019

Did someone test this?

@AdmiralCurtiss

This comment has been minimized.

Copy link
Contributor Author

AdmiralCurtiss commented Feb 16, 2019

Poking this one. Could someone else test this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment