Emulator Issues #9006
closed
Fifo saving is broken
0%
Description
FifoDataFile::PadFile() is meant to be a very simple function that extends a file's size by writing nul bytes. Unfortunately it was apparently written by someone who doesn't comprehend how fwrite works.
Depending on how much padding is being written (varies depending on the number of frames/number of memory updates being written), three things can happen: no error, unhandled segfault, or (my personal favourite) a segfault that gets eaten and results in a fifo data file with a valid header/frame info list but messed up fifo data/memory contents.
Updated by JMC4789 about 9 years ago
- Status changed from New to Accepted
I'm accepting this based on the fact that I seem to get garbage fifologs pretty often, and this sounds like a possibility to cause the issues.
Updated by JosJuice about 9 years ago
- Status changed from Accepted to Fix pending
- Milestone set to Current
- Regression changed from No to Yes
- Easy changed from No to Yes
Updated by delroth about 9 years ago
- Status changed from Fix pending to Accepted
- Assignee set to delroth
- Easy changed from Yes to No
Regression from 4.0-1127. I have a fix in PR3131.
(Didn't mean to switch the Easy bit — edited at the same time... meh.)
Updated by JosJuice about 9 years ago
- Status changed from Accepted to Fix pending
- Easy changed from No to Yes
Updated by tueidj about 9 years ago
The proposed patch is flawed.
fseek() can move the file pointer beyond the end of the file but it won't change the length unless data is written. If a subsequent fseek references SEEK_END any unwritten padding bytes will not be included. This is exactly what happens in FifoDataFile::WriteMemoryUpdates().
Updated by delroth about 9 years ago
Ah, didn't notice WriteMemoryUpdates was doing this kind of SEEK_END stuff. Indeed. That's why my PR says "untested" :)
I'll just go back to the pre-4.0-1127 implementation of fputc in a loop. Not great but at least it's safe. Not like this is in a hot path anyway...
Updated by tueidj about 9 years ago
Successfully saved a 1000 frame fifo log and was actually able to play it back (without Dolphin throwing >9000 unknown opcode dialogs) using the PR build.
A tip for those cringey "Dolphin will now most likely crash" pop-ups: add an abort button so we don't have to wade through dozens of repeated errors until the inevitable crash actually occurs. Even DOS had enough foresight to give the user a choice of "Abort, Retry, Ignore?"
Updated by JosJuice about 9 years ago
- Status changed from Fix pending to Fixed