3
\$\begingroup\$

This is a function that checks if a valid FAT12 image would be created. Since all the information to create it is available on the "boot sector", this is the parameter. I wanted the code to be pretty readable, but I may have overdone it.

fat.h

/* This file defines some FAT structures, and functions for validating a FAT12 * objects. */ #ifndef YMFAT12_FAT_H #define YMFAT12_FAT_H #include <stdio.h> /* The names are similar to the specs. */ /* BIOS parameter block (BPB) */ struct fat_bpb { unsigned short bytsPerSec; unsigned char secPerClus; unsigned short rsvdSecCnt; unsigned char numFATs; unsigned short rootEntCnt; unsigned short totSec16; unsigned char media; unsigned short FATSz16; unsigned short secPerTrk; unsigned short numHeads; unsigned long hiddSec; unsigned long totSec32; }; /* Extended BPB (EBPB) */ struct fat_ebpb { unsigned char drvNum; unsigned char reserved1; unsigned char bootSig; unsigned char volID; char volLab[11]; char filSysType[8]; }; struct fat_bootsector { unsigned char jmpBoot[3]; char oemName[8]; struct fat_bpb bpb; struct fat_ebpb ebpb; unsigned char code[448]; unsigned char signature_word[2]; }; /* Returns whether the bootsector information would make a valid FAT12 image. * @param [in] bs The bootsector with the sizes of the FAT12 * objects, and volume information. * @return Whether the FAT12 image would be valid. */ bool check_bs(struct fat_bootsector *bs); #endif /* YMFAT12_FAT_H */

fat.c

/* Implements what is declared on "fat.h". * * The "robustness principle" is generally used here: * >> Be conservative in what you send, be liberal in what you accept * It is mostly broken when compatibility with MS-DOS would restrict the specs. * * The specification used was from "August 30 2005" on * "https://academy.cba.mit.edu/classes/networking_communications/SD/FAT.pdf" * (accessed 2024/01/28) * When Wikipedia is mentioned the following article is being mentioned: * "https://en.wikipedia.org/wiki/Design_of_the_FAT_file_system" (accessed * 2024/01/28). * When OSDev is mentioned, the following article is being mentioned: * https://wiki.osdev.org/FAT (accessed 2024/01/28). * * Since values besides `char` can vary in length, the value size are checked * to be n-bytes long. This does assume 1 byte is 8-bits long, which is wild. */ #include "fat.h" #include <string.h> #include <limits.h> #include "error.h" #include "common.h" /* For the specs, a FAT is 12, 16 or 32 according to its number of clusters. * Different specs define different limits. The used spec defines 4084 for * FAT12, as mkfs.dos and Linux driver. The following limit was decided for the * following reasons. * 1. Cluster _values_ 0 and 1 are reserved, so 4096-2=4094 would be the limit. * 2. For Wikipedia, cluster values 0xFF0-0xFF5 are reserved or used, so * for portability the limit would be 4094-6=4088. * 3. For Wikipedia, cluster value 0xFF6 is reserved. Then the limit would be * 4088-1 = 4087. * 4. Cluster values 0xFF7-0xFFF have special meaning, so the limit would be * 4087-9=4078. * Q.E.D. */ #define FAT12_MAX_CLUSTERS 4078 static bool overflows3_add_ul( unsigned long n1, unsigned long n2, unsigned long n3 ) { if (n1 + n2 < n1) return false; return n1 + n2 + n3 < n1 + n2; } static inline bool overflows_mult_ul(unsigned long n1, unsigned long n2) { return n2 > ULONG_MAX / n1; } static inline bool is_16bit(unsigned short n) { return (n & 0xFFFF) == n; } static inline bool is_32bit(unsigned long n) { return (n & 0xFFFFFFFF) == n; } static inline bool is_lowercase_ascii(char n) { return n >= 0x61 && n <= 0x7A; } /* Whether is a valid 8.3 name */ static bool is_83name(const char *n) { const size_t sz = 11; size_t i; if (n[0] == 0x20) return false; for (i = 0; i < sz; i++) { if (is_lowercase_ascii(n[i])) return false; if (n[i] < 0x20) return false; if ( n[i] == 0x22 || n[i] == 0x2A || n[i] == 0x2B || n[i] == 0x2C || n[i] == 0x2E || n[i] == 0x2F || n[i] == 0x3A || n[i] == 0x3B || n[i] == 0x3C || n[i] == 0x3D || n[i] == 0x3E || n[i] == 0x3F || n[i] == 0x5B || n[i] == 0x5C || n[i] == 0x5D || n[i] == 0x7C) { return false; } } return true; } /* Checks if each field is within specs. */ static bool check_bs_fields(struct fat_bootsector *bs) { /* The function is long, but breaking it would not help, so I added * marks for what was being checked. */ /* jmpBoot */ /* For OSDev, Windows and OS X do not recognize the volume if the * bootjump does not follow the standard. */ if (bs->jmpBoot[0] == 0xEB) { if (bs->jmpBoot[2] != 0x90) return false; } else if (bs->jmpBoot[0] != 0xE9) return false; /* oemName does not need checking. It can be whatever. */ /* bytsPerSec */ if ( bs->bpb.bytsPerSec != 512 && bs->bpb.bytsPerSec != 1024 && bs->bpb.bytsPerSec != 2048 && bs->bpb.bytsPerSec != 4096) { return false; } /* secPerClus */ if ( bs->bpb.secPerClus != 1 && bs->bpb.secPerClus != 2 && bs->bpb.secPerClus != 4 && bs->bpb.secPerClus != 8 && bs->bpb.secPerClus != 16 && bs->bpb.secPerClus != 32 && bs->bpb.secPerClus != 64 && bs->bpb.secPerClus != 128) { return false; } /* rsvdSecCnt */ if (!is_16bit(bs->bpb.rsvdSecCnt)) return false; if (bs->bpb.rsvdSecCnt == 0) return false; /* numFATs */ /* Maybe only 1 and 2 values should be allowed. */ if (!is_16bit(bs->bpb.numFATs)) return false; if (bs->bpb.numFATs == 0) return false; /* rootEntCnt */ if (!is_16bit(bs->bpb.rootEntCnt)) return false; /* Specs does not exclude 0, but it is natural. */ if (bs->bpb.rootEntCnt == 0) return false; if ((long)bs->bpb.rootEntCnt * 32 % bs->bpb.bytsPerSec != 0) return false; /* totSec16 */ /* If it is 0, then totSec32 is used instead. */ if (!is_16bit(bs->bpb.totSec16)) return false; /* media */ /* Wikipedia asserts there is more. */ if ( bs->bpb.media != 0xF0 && bs->bpb.media != 0xF8 && bs->bpb.media != 0xF9 && bs->bpb.media != 0xFA && bs->bpb.media != 0xFB && bs->bpb.media != 0xFC && bs->bpb.media != 0xFD && bs->bpb.media != 0xFE && bs->bpb.media != 0xFF) { return false; } /* FATSz16 */ if (!is_16bit(bs->bpb.media)) return false; /* Specs does not exclude 0, but it is natural. */ if (bs->bpb.FATSz16 == 0) return false; /* secPerTrk */ if (!is_16bit(bs->bpb.secPerTrk)) return false; /* numHeads */ if (!is_16bit(bs->bpb.numHeads)) return false; /* Wikipedia says MS-DOS had a bug for CHS with 256 heads up to * including 7.1. So most BIOSes do not support more than 255 heads. */ if (bs->bpb.numHeads >= 256) return false; /* hiddSec */ if (!is_16bit(bs->bpb.hiddSec)) return false; /* totSec32 */ if (!is_32bit(bs->bpb.totSec32)) return false; if (bs->bpb.totSec32 == 0 && bs->bpb.totSec16 == 0) return false; /* drvNum */ if (bs->ebpb.drvNum != 0x00 && bs->ebpb.drvNum != 0x80) return false; /* reserved1 */ if (bs->ebpb.reserved1 != 0) return false; /* bootSig */ /* For Wikipedia, it can also be 0x28, in which case the following * fields are not used. */ if (bs->ebpb.bootSig != 0x29) return false; /* volID does not need checking. */ /* volLab */ /* The specs do not affirm its format, but the examples align with * 8.3 names, and that it should be same as the volume file. */ if (!is_83name(bs->ebpb.volLab)) return false; /* filSysType */ /* Since it is informational, it could be allowed to be whatever. */ if (memcmp(bs->ebpb.filSysType, "FAT12 ", 8) != 0) return false; /* signature_word */ /* For Wikipedia, MS-DOS <3.3 checked if it was 0x55 0xAA. */ if (bs->signature_word[0] != 0x55 || bs->signature_word[1] != 0xAA) return false; return true; } bool check_bs(struct fat_bootsector *bs) { unsigned long fatTot, res, fat, rDir, data; if (!check_bs_fields(bs)) return false; /* Some ugly overflow checks were needed. */ fatTot = bs->bpb.totSec16 == 0 ? bs->bpb.totSec32 : bs->bpb.totSec16; res = (unsigned long)bs->bpb.rsvdSecCnt * bs->bpb.bytsPerSec; fat = (unsigned long)bs->bpb.numFATs * bs->bpb.FATSz16; /* On section 4 of the spec, "The size of the FAT is limited to 6K * sectors on volumes formatted FAT12". */ if (fat > 6 * 1028) return false; if (overflows_mult_ul(fat, bs->bpb.bytsPerSec)) return false; fat *= bs->bpb.bytsPerSec; rDir = (unsigned long)bs->bpb.rootEntCnt * 32; if (res + fat + rDir > fatTot) return false; if (overflows3_add_ul(res, fat, rDir)) return false; data = fatTot - (res + fat + rDir); if (data > FAT12_MAX_CLUSTERS) return false; return true; }
\$\endgroup\$
0

    1 Answer 1

    Reset to default
    3
    \$\begingroup\$

    fat.h has no need to #include <stdio.h>, so remove that.

    However, it does require <stdbool.h>.

    I believe that the on-disk structures have specific sizes, rather than depending on the platform's char, short and long. If I'm right, we should be using fixed-width types from <stdint.h>. We might also need to take measures to avoid being tripped up by structure padding.

    Instead of using preprocessor #define FAT12_MAX_CLUSTERS 4078, prefer to use an integer constant.

    I don't like the dependence of overflows3_add_ul on unsigned overflow, even though that's well-defined. It would be more transparent if it tested against ULONG_MAX as its multiplicative sister does:

     return n2 > ULONG_MAX - n1 || n3 > ULONG_MAX - n2 - n1;

    The is_lowercase_ascii() test might need a comment to say why we're using numeric values for the comparison (i.e. to ensure that the code is still correct when run in a non-ASCII environment) and prevent a future maintainer erroneously "correcting" to 'a' and 'z'.

    I'm not convinced of the correctness of is_83name(), having read the description of directory table. I think it's better to whitelist the allowed characters, rather than attempting to enumerate what's not allowed. And we do we need to handle 0x05/0xE5 in the first position?

    Testing for exact powers of two might be easier with the common x & (x-1) bit-hack, plus a simple range test.

    is_16bit(bs->bpb.hiddSec) truncates the unsigned long to unsigned short - we need to widen the function's formal arguments, or perform an additional test before truncating with a cast.

    There's a whole series of tests for non-zero 16-bit values: we might be better off implementing bool is_in_range(unsigned long value, unsigned long min, unsigned long max) and using that throughout instead. A reasonable optimiser knows that x & 0xffff != x and x <= 0xffff are equivalent.

    We have redundancy here:

     if (!is_16bit(bs->bpb.numHeads)) return false; if (bs->bpb.numHeads >= 256) return false;

    The first test can be eliminated.

    I don't like the cast from unsigned to signed here:

     if ((long)bs->bpb.rootEntCnt * 32 % bs->bpb.bytsPerSec != 0)

    unsigned long would be a better choice.

    I don't particularly like the magic-number length here:

     if (memcmp(bs->ebpb.filSysType, "FAT12 ", 8) != 0)

    Consider using sizeof bs->ebpb.filSysType in place of 8 there for clarity.

    \$\endgroup\$
    1
    • \$\begingroup\$Thank you, Toby! I really liked the idea for a rage function. I used that definition on is_83name() because the specs define that way, but I should prohibit ' ' anyway, since programs did not expect spaces. And I had not thought of that about is_lowercase_ascii(), though I have never experienced a non-ASCII environment.\$\endgroup\$
      – Schilive
      CommentedFeb 1, 2024 at 17:42

    Start asking to get answers

    Find the answer to your question by asking.

    Ask question

    Explore related questions

    See similar questions with these tags.