Take some minor changes from OpenBSD's fsck_msdos. In particular: Revision 1.13 (boot.c) Check reads and lseek correctly for unsigned return Revision 1.17 (fat.c) - Partial use calloc() to avoid malloc(n * m) overflow How-To-Repeat: Unfortunately someone has to review this carefully since fsck_msdosfs doesn't seem to work properly on my system: mogwai# fsck_msdosfs -p /dev/da0s1 Can't open (No such file or directory) /dev/ad0s1: UNEXPECTED INCONSISTENCY; RUN fsck_msdosfs MANUALLY.
On Wed, Jan 6, 2010 at 5:50 PM, Pedro Giffuni <giffunip@tutopia.com> wrote: [snip] > diff -ru fsck_msdosfs.orig/boot.c fsck_msdosfs/boot.c > --- fsck_msdosfs.orig/boot.c =A0 =A02010-01-06 11:07:24.000000000 +0000 > +++ fsck_msdosfs/boot.c 2010-01-06 11:19:21.000000000 +0000 > @@ -55,9 +55,9 @@ > =A0 =A0 =A0 =A0u_char block[DOSBOOTBLOCKSIZE]; > =A0 =A0 =A0 =A0u_char fsinfo[2 * DOSBOOTBLOCKSIZE]; > =A0 =A0 =A0 =A0u_char backup[DOSBOOTBLOCKSIZE]; > - =A0 =A0 =A0 int ret =3D FSOK; > + =A0 =A0 =A0 int n, ret =3D FSOK; > > - =A0 =A0 =A0 if (read(dosfs, block, sizeof block) < sizeof block) { > + =A0 =A0 =A0 if ((n=3D(read(dosfs, block, sizeof block) < sizeof block))= =3D=3D -1 || n !=3D sizeof block) { > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0perror("could not read boot block"); > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return FSFATAL; > =A0 =A0 =A0 =A0} This does not look right, you probably want: if ((n =3D read(dosfs, block, sizeof(block))) =3D=3D -1 || n !=3D sizeof(b= lock)) Cheers, Antoine
----- Original Message ---- ... > > This does not look right, you probably want: > if ((n = read(dosfs, block, sizeof(block))) == -1 || n != sizeof(block)) > Hmm... It was taken as-is from OpenBSD's CVS http://www.openbsd.org/cgi-bin/cvsweb/src/sbin/fsck_msdos/boot.c.diff?r1=1.12;r2=1.13 but I have no objection with adding the parenthesis. cheers, Pedro.
On Wed, Jan 6, 2010 at 7:05 PM, Pedro F. Giffuni <giffunip@tutopia.com> wrote: >> This does not look right, you probably want: >> if ((n = read(dosfs, block, sizeof(block))) == -1 || n != sizeof(block)) >> > > Hmm... It was taken as-is from OpenBSD's CVS > > http://www.openbsd.org/cgi-bin/cvsweb/src/sbin/fsck_msdos/boot.c.diff?r1=1.12;r2=1.13 > > but I have no objection with adding the parenthesis. My problem was with the parenthesis between "n=" and "read" in your patch. Cheers, Antoine
----- Original Message ---- ... > > My problem was with the parenthesis between "n=" and "read" in your patch. > Ooops.. my bad, sorry! New patch attached.
As Bruce Evans has kindly noted, the OpenBSD changes are plagued with style errors so we should use the NetBSD changes instead. This patch is bigger but cleans real bugs. In general: - fix sign-compare issues - Move to 2 clause license, approved by Wolfgang Solfrank In boot.c: - Change mismatch of bytes 11 to 90 to be a warning, not an error - print out the values of the bytes that do not match. - Add comment explaining that there is no documented rationale for the check. - removes unused ctype.h header in dir.c: - use bounded string op - the root directory of non fat32 filesystems is stored in a special area. a couple of corner cases can cause it to fail to write out that area after it performs repairs. in fat.c: - don't leak memory on allocation failure. - Do not crash when boot->FSNext contains garbage (i.e. -1). - don't use uint32_t when you mean size_t.
A new, and hopefully final, update: -Added a fix to a bug coverity found on NetBSD. -Removed bogus check that attempted to clear the sector after FSInfo in a valid FAT32 filesystem.
(I spoke too soon) Remove unused variable len: This is used by NetBSD in error reports but we don't use it.
Ugh... Fix misplaced cast.
While here,.. - clarify a bit the FAT header - ansify a couple of functions
In addition to the NetBSD fixes, this patch renames the fields in the bootblock struct in preparation for a header unification patch based on the msdosfs headers. At least I am sure we are not ruining filesystems: mogwai# fsck -t msdosfs /dev/da0s1 ** /dev/da0s1 ** Phase 1 - Read and Compare FATs ** Phase 2 - Check Cluster Chains ** Phase 3 - Checking Directories ** Phase 4 - Checking for Lost Files 2310 files, 2306176 free (576544 clusters)
As a reminder before this gets lost ... After some private discussion with bde@ and kib@ here is a patch with cleanups for fsck_msdos.
Responsible Changed From-To: freebsd-bugs->brian Take
State Changed From-To: open->patched Applied to head (r209364) with a few style(9) tweaks. I'll MFC in three weeks if there are no complaints.
Author: brian Date: Sun Jun 20 09:40:54 2010 New Revision: 209364 URL: http://svn.freebsd.org/changeset/base/209364 Log: Fix some style(9), although there's a lot more issues here. Fix some casting errors. PR: 142384 Submitted by: giffunip at tutopia dot com Obtained from: NetBSD MFC after: 3 weeks Modified: head/sbin/fsck_msdosfs/Makefile head/sbin/fsck_msdosfs/boot.c head/sbin/fsck_msdosfs/check.c head/sbin/fsck_msdosfs/dir.c head/sbin/fsck_msdosfs/fat.c Modified: head/sbin/fsck_msdosfs/Makefile ============================================================================== --- head/sbin/fsck_msdosfs/Makefile Sun Jun 20 08:48:30 2010 (r209363) +++ head/sbin/fsck_msdosfs/Makefile Sun Jun 20 09:40:54 2010 (r209364) @@ -9,6 +9,6 @@ MAN= fsck_msdosfs.8 SRCS= main.c check.c boot.c fat.c dir.c fsutil.c CFLAGS+= -I${FSCK} -WARNS?= 0 +WARNS?= 2 .include <bsd.prog.mk> Modified: head/sbin/fsck_msdosfs/boot.c ============================================================================== --- head/sbin/fsck_msdosfs/boot.c Sun Jun 20 08:48:30 2010 (r209363) +++ head/sbin/fsck_msdosfs/boot.c Sun Jun 20 09:40:54 2010 (r209364) @@ -48,13 +48,14 @@ readboot(int dosfs, struct bootblock *bo int ret = FSOK; int i; - if ((size_t)read(dosfs, block, sizeof block) != sizeof block) { + if (read(dosfs, block, sizeof block) != sizeof block) { perror("could not read boot block"); return FSFATAL; } if (block[510] != 0x55 || block[511] != 0xaa) { - pfatal("Invalid signature in boot block: %02x%02x", block[511], block[510]); + pfatal("Invalid signature in boot block: %02x%02x", + block[511], block[510]); return FSFATAL; } @@ -72,8 +73,10 @@ readboot(int dosfs, struct bootblock *bo boot->bpbFATsmall = block[22] + (block[23] << 8); boot->SecPerTrack = block[24] + (block[25] << 8); boot->bpbHeads = block[26] + (block[27] << 8); - boot->bpbHiddenSecs = block[28] + (block[29] << 8) + (block[30] << 16) + (block[31] << 24); - boot->bpbHugeSectors = block[32] + (block[33] << 8) + (block[34] << 16) + (block[35] << 24); + boot->bpbHiddenSecs = block[28] + (block[29] << 8) + + (block[30] << 16) + (block[31] << 24); + boot->bpbHugeSectors = block[32] + (block[33] << 8) + + (block[34] << 16) + (block[35] << 24); boot->FATsecs = boot->bpbFATsmall; @@ -97,10 +100,9 @@ readboot(int dosfs, struct bootblock *bo boot->bpbFSInfo = block[48] + (block[49] << 8); boot->bpbBackup = block[50] + (block[51] << 8); - if (lseek(dosfs, boot->bpbFSInfo * boot->bpbBytesPerSec, SEEK_SET) - != boot->bpbFSInfo * boot->bpbBytesPerSec - || read(dosfs, fsinfo, sizeof fsinfo) - != sizeof fsinfo) { + if (lseek(dosfs, boot->bpbFSInfo * boot->bpbBytesPerSec, + SEEK_SET) != boot->bpbFSInfo * boot->bpbBytesPerSec + || read(dosfs, fsinfo, sizeof fsinfo) != sizeof fsinfo) { perror("could not read fsinfo block"); return FSFATAL; } @@ -124,7 +126,8 @@ readboot(int dosfs, struct bootblock *bo fsinfo[0x3fc] = fsinfo[0x3fd] = 0; fsinfo[0x3fe] = 0x55; fsinfo[0x3ff] = 0xaa; - if (lseek(dosfs, boot->bpbFSInfo * boot->bpbBytesPerSec, SEEK_SET) + if (lseek(dosfs, boot->bpbFSInfo * + boot->bpbBytesPerSec, SEEK_SET) != boot->bpbFSInfo * boot->bpbBytesPerSec || write(dosfs, fsinfo, sizeof fsinfo) != sizeof fsinfo) { @@ -144,7 +147,8 @@ readboot(int dosfs, struct bootblock *bo + (fsinfo[0x1ef] << 24); } - if (lseek(dosfs, boot->bpbBackup * boot->bpbBytesPerSec, SEEK_SET) + if (lseek(dosfs, boot->bpbBackup * boot->bpbBytesPerSec, + SEEK_SET) != boot->bpbBackup * boot->bpbBytesPerSec || read(dosfs, backup, sizeof backup) != sizeof backup) { perror("could not read backup bootblock"); @@ -172,11 +176,10 @@ readboot(int dosfs, struct bootblock *bo /* Check backup bpbFSInfo? XXX */ } - boot->ClusterOffset = (boot->bpbRootDirEnts * 32 + boot->bpbBytesPerSec - 1) - / boot->bpbBytesPerSec - + boot->bpbResSectors - + boot->bpbFATs * boot->FATsecs - - CLUST_FIRST * boot->bpbSecPerClust; + boot->ClusterOffset = (boot->bpbRootDirEnts * 32 + + boot->bpbBytesPerSec - 1) / boot->bpbBytesPerSec + + boot->bpbResSectors + boot->bpbFATs * boot->FATsecs - + CLUST_FIRST * boot->bpbSecPerClust; if (boot->bpbBytesPerSec % DOSBOOTBLOCKSIZE != 0) { pfatal("Invalid sector size: %u", boot->bpbBytesPerSec); @@ -191,7 +194,8 @@ readboot(int dosfs, struct bootblock *bo boot->NumSectors = boot->bpbSectors; } else boot->NumSectors = boot->bpbHugeSectors; - boot->NumClusters = (boot->NumSectors - boot->ClusterOffset) / boot->bpbSecPerClust; + boot->NumClusters = (boot->NumSectors - boot->ClusterOffset) / + boot->bpbSecPerClust; if (boot->flags&FAT32) boot->ClustMask = CLUST32_MASK; Modified: head/sbin/fsck_msdosfs/check.c ============================================================================== --- head/sbin/fsck_msdosfs/check.c Sun Jun 20 08:48:30 2010 (r209363) +++ head/sbin/fsck_msdosfs/check.c Sun Jun 20 09:40:54 2010 (r209364) @@ -98,7 +98,7 @@ checkfilesys(const char *fname) } if (boot.ValidFat < 0) - for (i = 1; i < (int)boot.bpbFATs; i++) { + for (i = 1; i < boot.bpbFATs; i++) { struct fatEntry *currentFat; mod |= readfat(dosfs, &boot, i, ¤tFat); Modified: head/sbin/fsck_msdosfs/dir.c ============================================================================== --- head/sbin/fsck_msdosfs/dir.c Sun Jun 20 08:48:30 2010 (r209363) +++ head/sbin/fsck_msdosfs/dir.c Sun Jun 20 09:40:54 2010 (r209364) @@ -242,7 +242,8 @@ resetDosDirSection(struct bootblock *boo memset(rootDir, 0, sizeof *rootDir); if (boot->flags & FAT32) { - if (boot->bpbRootClust < CLUST_FIRST || boot->bpbRootClust >= boot->NumClusters) { + if (boot->bpbRootClust < CLUST_FIRST || + boot->bpbRootClust >= boot->NumClusters) { pfatal("Root directory starts with cluster out of range(%u)", boot->bpbRootClust); return FSFATAL; @@ -356,7 +357,8 @@ removede(int f, struct bootblock *boot, pwarn("Invalid long filename entry for %s\n", path); break; case 1: - pwarn("Invalid long filename entry at end of directory %s\n", path); + pwarn("Invalid long filename entry at end of directory %s\n", + path); break; case 2: pwarn("Invalid long filename entry for volume label\n"); @@ -418,7 +420,8 @@ checksize(struct bootblock *boot, struct cl_t cl; u_int32_t sz = 0; - for (cl = dir->head; (sz += boot->ClusterSize) < dir->size;) + for (cl = dir->head; (sz += boot->ClusterSize) < + dir->size;) cl = fat[cl].next; clearchain(boot, fat, fat[cl].next); fat[cl].next = CLUST_EOF; @@ -462,7 +465,8 @@ readDosDirSection(int f, struct bootbloc do { if (!(boot->flags & FAT32) && !dir->parent) { last = boot->bpbRootDirEnts * 32; - off = boot->bpbResSectors + boot->bpbFATs * boot->FATsecs; + off = boot->bpbResSectors + boot->bpbFATs * + boot->FATsecs; } else { last = boot->bpbSecPerClust * boot->bpbBytesPerSec; off = cl * boot->bpbSecPerClust + boot->ClusterOffset; @@ -547,7 +551,8 @@ readDosDirSection(int f, struct bootbloc } lidx = *p & LRNOMASK; t = longName + --lidx * 13; - for (k = 1; k < 11 && t < longName + sizeof(longName); k += 2) { + for (k = 1; k < 11 && t < longName + + sizeof(longName); k += 2) { if (!p[k] && !p[k + 1]) break; *t++ = p[k]; Modified: head/sbin/fsck_msdosfs/fat.c ============================================================================== --- head/sbin/fsck_msdosfs/fat.c Sun Jun 20 08:48:30 2010 (r209363) +++ head/sbin/fsck_msdosfs/fat.c Sun Jun 20 09:40:54 2010 (r209364) @@ -87,7 +87,8 @@ checkdirty(int fs, struct bootblock *boo goto err; } - if (read(fs, buffer, boot->bpbBytesPerSec) != boot->bpbBytesPerSec) { + if ((size_t)read(fs, buffer, boot->bpbBytesPerSec) != + boot->bpbBytesPerSec) { perror("Unable to read FAT"); goto err; } _______________________________________________ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
State Changed From-To: patched->closed Merged to stable/8, r209914