search

deater / dos33fsprogs

Tools for manipulating Apple II dos33 filesystems
http://www.deater.net/weave/vmwprod/apple/dos33fs.html
Other
142 stars 23 forks source link

Fix critical bug with saving that prevents BLOAD/BRUN working #15

Closed nickd4 closed 2 years ago

nickd4 commented 2 years ago

I have been making some patches to an Apple game which is a large binary executable that you run with BRUN. I loaded my patched version onto a disk using a command like this:

./dos33 star_blazer.dsk SAVE B star_blazer_patched.bin "STAR BLAZER PATCHED"

However, to my dismay the program didn't run correctly. The graphics was messed up, and since I knew the latter part of the executable consists of graphics data, I suspected that the file was not being completely loaded into memory.

After in-depth investigation I found that I could reproduce the problem using the original executable without my patches:

./dos33 star_blazer.dsk LOAD "STAR BLAZER" star_blazer.bin
./dos33 star_blazer.dsk SAVE B star_blazer.bin "STAR BLAZER1"

This results in a an executable that can't be run with BRUN anymore. But you can still read back the executable using dos33 and it is not corrupted. So the issue seemed to be something to do with the file metadata rather than the file data itself.

To try to see what DOS 3.3 was seeing, I copied the file within DOS 3.3 onto another disk using FID, and interestingly, after copying with FID the copied executable could be run correctly with BRUN. (It also still reads back correctly with dos33).

The problem turned out to be in the bytes TSL_OFFSET_L and TSL_OFFSET_H. I made dos33 print out these bytes as it read each TS-list sector and for the good disk, and it printed 00 00 and 7a 00 on the good executable but 00 00 and 00 00 on the bad executable. So basically the offset wasn't being set correctly in the last TS-list sector. And this would only affect executables that are > 122 sectors (15.6 kbytes) in size, so that's probably why the bug hasn't been noticed until now.

The existing algorithm was setting the offset in the TS-list when it filled in the link to the next sector, which explains why the last TS-list sector wasn't getting it, because the link in the last TS-list sector is never filled in. So I moved this code to the point where it allocates a TS-list sector. Also, there was a spurious multiply by 0x100 which I took out so it writes the correct value.

I can confirm that this fixes the problem for me. It's a critical bug so I hope it is merged promptly. Thanks a lot for your work.

Update: I also added a .gitignore which I need for my project. It would certainly not hurt to merge this file in as well.

deater commented 2 years ago

thanks, took me a while to figure out what's going on, as from what I can tell DOS3.3 itself ignores the field we're talking about.

I merged the patches manually.

nick-lifx commented 2 years ago

Cool. I am not 100% sure which DOS 3.3 I used for this test -- probably the one that was already on the game disk, which might not be the best reference. I could try again with the 1984 latest version of DOS 3.3. But I think it must be checking this field. At any rate, with this patch we can be confident that dos33 utility leaves the disk in a similar state to how FID would at Apple side.