Check if options are valid to create FAT12 image

Question

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;
}

Answer 1

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.

• Schilive: 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.