joncampbell123 / dosbox-x

DOSBox-X fork of the DOSBox project
GNU General Public License v2.0
2.81k stars 383 forks source link

FindFirst (int21,4e) can invalidate handles stored in other DTA blocks #4904

Open JonnyH opened 8 months ago

JonnyH commented 8 months ago

Describe the bug

I'm looking at a sporadic issue running X:Com Apocalypse, and thing I've found an issue in how dosbox handles multiple "active" DTA blocks, the end result being a FindFile incorrectly fails to find an existing file, and the game crashes (specifically, shows a "Please insert the CD" screen).

So XCom: Apocalypse was compiled using the watcom C compiler, and seems to use it's libc file handling utilities on DOS.

It seems use a wrapper for the find file functions (FindFirst, int 21,4e and FindNext, int21,4f) that first call SetDTA (int21,1a) to keep find contexts separate.

This app searches for files on both the HDD and CD, but seems to interleave the searches, so you end up with something like the following pseudocode:

SetDTA(DTA_A)
FindFirst("pattern")
SetDTA(DTA_B)
FindFirst("pattern")

while (searching) {
   SetDTA(DTA_A);
   error = FindNext();
   if (error) return error;
   SetDTA(DTA_B);
   error = FindNext();
   if (error) return error;
}

As far as I can see from the dos documentation I can find, this is acceptable - though both calls are interleaved, each should use a completely separate DTA, and no other calls modify the state of the filesystem, so it should never fail to find an existing file. Even if it might be a bit of an odd pattern.

Now the problem seems to be with Dosbox is when we introduce the DOS_Drive_Cache, and the second FindFirst (to DTA_B above) causes the cache to be invalidated - in bool DOS_Drive_Cache::FindFirst(char* path, uint16_t& id):

   if (local_findcounter == MAX_OPENDIRS) { //Here is the reset from above.
       // no free slot found...
       LOG(LOG_DOSMISC,LOG_ERROR)("DIRCACHE: FindFirst/Next: All slots full. Resetting");
       // Clear the internal list then.
       dirFindFirstID = 0;
       this->nextFreeFindFirst = 1; //the next free one after this search
       for(Bitu n=0; n<MAX_OPENDIRS;n++) { 
           // Clear and reuse slot
           DeleteFileInfo(dirFindFirst[n]);
           dirFindFirst[n]=0;
       }

   }       

This invalidates all file handles - including the one being stored in DTA_A, so when the FiindNext() call happens in bool DOS_Drive_Cache::FindNext(uint16_t id, char* &result, char* &lresult) the error path gets hit:

    // out of range ?
    if ((id>=MAX_OPENDIRS) || !dirFindFirst[id]) {
        LOG(LOG_DOSMISC,LOG_ERROR)("DIRCACHE: FindFirst/Next failure : ID out of range: %04X",id);
        return false;
    }

and the call fails without finding an otherwise existent file.

This seems to show up as the following in a log with Files I/O logging enabled:

3197707600       FILES:file search attributes 1 name MAPS/21APPL/MAPUNITS/SGROUND.PCK
3197707600 ERROR DOSMISC:DIRCACHE: FindFirst/Next: All slots full. Resetting
3197714061       FILES:file search attributes 1 name MAPS/21APPL/MAPUNITS/SGROUND.PCK
3197717027       FILES:file open command 0 file MAPS/21APPL/MAPUNITS/SGROUND.PCK
3197743627       FILES:file open command 1 file scenario/SGROUND.PCK
3197746049       FILES:file create attributes 0 file scenario/SGROUND.PCK
3197769443 ERROR DOSMISC:DIRCACHE: FindFirst/Next failure : ID out of range: 07B7

Steps to reproduce the behaviour

The repro case is rather hard - it seems to be "Play XCom: Apocalypse in one session for multiple hours without quitting", and you get a "Please Insert The CD" error window show up when trying to start a new battle mission.

But I believe using the pseudocode above, a tighter example could be made - all you need to do is ensure that enough directories are open to fill the cache and cause a flush while there's still another "live" DTA somewhere.

What operating system(s) this bug have occurred on?

Seen on Windows 10, Windows 11 and Linux amd64

What version(s) of DOSBox-X have this bug?

Latest master, occurs at least back to original dosbox 0.72

Have you checked that no similar bug report(s) exist?

Code of Conduct & Contributing Guidelines

tazok81 commented 7 months ago

+1:)

ltning commented 2 weeks ago

Is it OK to comment that the time spent on the "Play XCom: Apocalypse in one session for multiple hours without quitting" reproduction case here, accounting for all the different platforms and dosbox-versions this has been confirmed on, must be an envieable amount? Kudos.