joncampbell123 / dosbox-x

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

File corruption on image files (when installing windows 3.1x, since dosbox-x v82.23 untill now) #2382

Closed SnikoLoft closed 3 years ago

SnikoLoft commented 3 years ago

Describe the bug

After creating an image file (i.e. imgmake file.img -t hd_250) and installing Windows 3.1 or 3.11 or WfW 3.11 with rebooting for installing s3-drivers and video for windows, After some reboots After inserting program icons I get the message, that one or more program group files are corrupt. The corruption starts, when a program modifies the program group or I change the color depth of the desktop.

I find the same problem in #1758 "WfW 3.11 - corrupted progman GRPs when using imgmount + internal shell + S3 driver" with english and german versions of win 3.x.

I loaded the latest dosbox-x source code and removed the discussed line (drive_fat.cpp) if (sizedec == 0) { curSectOff = 0; goto finalizeWrite; } comment of joncampbell123 compiled and the problem was gone.

I think, this line is for prevent allocating an extra (empty) hard disk sector, but sometimes it creates a file position problem: file pointer position wrong by one byte...

First Solution: deleting the existing line if (sizedec == 0) { curSectOff = 0; goto finalizeWrite; }

Or this second solution (or similar) maybe helps, to keep the right file position, by prevent only the write of the extra file sector operation.

bool fatFile::Write(const ... bool bWrite = true; ..... // if (sizedec == 0) { curSectOff = 0; goto finalizeWrite; } if (sizedec == 0) bWrite = false; ..... ~~ }~~ // if(curSectOff>0 && loadedSector) myDrive->writeSector(currentSector, sectorBuffer); if(curSectOff>0 && loadedSector && bWrite) myDrive->writeSector(currentSector, sectorBuffer);

With both solutions, the installation and usage of the WIN3.x system is fine.

Maybe this is a reason for more random failures with other installations...

rderooy commented 3 years ago

Please either use 0.83.10 or compile the latest code. There have been some fixes applied in the last days that are needed for installing Win9x and may also effect Win 3.1x

SnikoLoft commented 3 years ago

I used the latest code from today march 18th. Also I used the Sourcecode 82.20 (Okay) and v82.22 (Not Okay). All (?) versions after v82.20/21 include v83.10/11 have the same problems. If I don't use an hard-disk.img file and use a folder of my Windows-10 host instead, then there is no error noticeable.

Used Windows x64 SDL1 & SDL2 and x86 versions of dosbox-x

Wengier commented 3 years ago

@SnikoLoft Thanks for reporting the said issue. Since you appear to already know how to fix it, maybe you can send a PR directly? Thanks.

SnikoLoft commented 3 years ago

@Wengier OK, I will try it. I'm not familar to use git or similar to make a pull request, so give me this weekend to double check the fix and make the PR.

SnikoLoft commented 3 years ago

@Wengier After a weekend of learning, debugging, and testing I found the solution.

The problem with the corrupt windows 3.x group files was an fix from joncampbell (https://github.com/joncampbell123/dosbox-x/issues/1758#issuecomment-665193066).

But this fix broke the weird special creation of group files: Here an example of inserting a new program icon (i.e. old file length 8000Bytes, now extend with 400 Bytes...)

After my debugging session I found a secure solution: The fix from joncampbell has to move two lines downwards, direct before the appendCluster command. So his fix is working further on AND the weird group file handling is OK, and I think, it may fix some more erratic file corruption incidents...

But I don't get the 'GIT' thing in my head... Please, please add my little fix by yourself...

// 1. Search in files for   (sizedec == 0)
// You will find it in drive_fat.cpp

// 2. DELETE THE LINE:
//    if(sizedec == 0) { curSectOff = 0; goto finalizeWrite; }

      currentSector = myDrive->getAbsoluteSectFromBytePos(firstCluster, seekpos);
      if(currentSector == 0) {
// 3. INSERT NEXT LINE HERE:
        if(sizedec == 0)   goto finalizeWrite;
        /* EOC reached before EOF - try to increase file allocation */
    myDrive->appendCluster(firstCluster);
SnikoLoft commented 3 years ago

I've made a Pull Request for this. I edited the file online in the webbrowser...

Wengier commented 3 years ago

Thanks for the fix (PR #2390).