Bug in BPB validation

[dbalsom]

        if (self.total_sectors_16 == 0) == (self.total_sectors_32 == 0) {
            error!("Invalid BPB (total_sectors_16 or total_sectors_32 should be non-zero)");
            return Err(Error::CorruptedFileSystem);
        }

I believe this should be

        if (self.total_sectors_16 == 0) && (self.total_sectors_32 == 0) {
            error!("Invalid BPB (total_sectors_16 or total_sectors_32 should be non-zero)");
            return Err(Error::CorruptedFileSystem);
        }

Otherwise it will cause a failure if both total_sectors_16 and total_sectors_32 are nonzero, which is the case on many DOS disk images

[rafalh]

FAT specification says:

BPB_TotSec16:

This field can be 0; if it is 0, then BPB_TotSec32
must be non-zero. For FAT32 volumes, this field
must be 0.
For FAT12 and FAT16 volumes, this field contains
the sector count, and BPB_TotSec32 is 0 if the
total sector count “fits” (is less than 0x10000).

BPB_TotSec32:

This field can be 0; if it is 0, then BPB_TotSec16
must be non-zero. For FAT12/FAT16 volumes, this
field contains the sector count if BPB_TotSec16 is
0 (count is greater than or equal to 0x10000).
For FAT32 volumes, this field must be non-zero.

so for FAT32 BPB_TotSec16 should always be zero and for FAT12 and FAT16 it should only be non-zero if sector count is < 0x10000.

Well, docs are not fully specific if BPB_TotSec32 must be zero if sector count is < 0x10000, so maybe it should be changed. I wonder if all modern tools agree on those fields.

Do I understand it right that you tried to read some old image and encouraged this problem? What image it was?

[dbalsom]

disk.zip

here is an example. there are very many dos games that fall into this pattern

it opens with WinImage and DiskImageTool (https://github.com/Digitoxin1/DiskImageTool)