svn commit: r348074 - head/sbin/fsck_ffs
Kirk McKusick
mckusick at FreeBSD.org
Tue May 21 22:24:39 UTC 2019
Author: mckusick
Date: Tue May 21 22:24:38 2019
New Revision: 348074
URL: https://svnweb.freebsd.org/changeset/base/348074
Log:
This revision began as a simple change to eliminate an uninitialized warning
found by Coverity. However, upon closer inspection the implementation of
fsck_ffs's fsck_readdir() and dircheck() functions is both nearly impossible
to follow and fails to check / fix directories in several cases. So, this
revision is an entire rewrite of these two functions to clarify what they
are doing and also to get something that works properly.
Referred by: cem
Reviewed by: kib, David G Lawrence
MFC after: 3 days
CID 1401317: namlen may be used uninitialized
Modified:
head/sbin/fsck_ffs/dir.c
Modified: head/sbin/fsck_ffs/dir.c
==============================================================================
--- head/sbin/fsck_ffs/dir.c Tue May 21 22:17:00 2019 (r348073)
+++ head/sbin/fsck_ffs/dir.c Tue May 21 22:24:38 2019 (r348074)
@@ -61,7 +61,7 @@ static struct dirtemplate dirhead = {
};
static int chgino(struct inodesc *);
-static int dircheck(struct inodesc *, struct direct *);
+static int dircheck(struct inodesc *, struct bufarea *, struct direct *);
static int expanddir(union dinode *dp, char *name);
static void freedir(ino_t ino, ino_t parent);
static struct direct *fsck_readdir(struct inodesc *);
@@ -139,78 +139,70 @@ dirscan(struct inodesc *idesc)
}
/*
- * get next entry in a directory.
+ * Get and verify the next entry in a directory.
+ * We also verify that if there is another entry in the block that it is
+ * valid, so if it is not valid it can be subsumed into the current entry.
*/
static struct direct *
fsck_readdir(struct inodesc *idesc)
{
struct direct *dp, *ndp;
struct bufarea *bp;
- long size, blksiz, fix, dploc;
- int dc;
+ long size, blksiz, subsume_ndp;
+ subsume_ndp = 0;
blksiz = idesc->id_numfrags * sblock.fs_fsize;
+ if (idesc->id_filesize <= 0 || idesc->id_loc >= blksiz)
+ return (NULL);
bp = getdirblk(idesc->id_blkno, blksiz);
- if (idesc->id_loc % DIRBLKSIZ == 0 && idesc->id_filesize > 0 &&
- idesc->id_loc < blksiz) {
- dp = (struct direct *)(bp->b_un.b_buf + idesc->id_loc);
- if ((dc = dircheck(idesc, dp)) > 0) {
- if (dc == 2) {
- /*
- * dircheck() cleared unused directory space.
- * Mark the buffer as dirty to write it out.
- */
- dirty(bp);
- }
- goto dpok;
- }
- if (idesc->id_fix == IGNORE)
- return (0);
- fix = dofix(idesc, "DIRECTORY CORRUPTED");
- bp = getdirblk(idesc->id_blkno, blksiz);
- dp = (struct direct *)(bp->b_un.b_buf + idesc->id_loc);
- dp->d_reclen = DIRBLKSIZ;
- dp->d_ino = 0;
- dp->d_type = 0;
- dp->d_namlen = 0;
- dp->d_name[0] = '\0';
- if (fix)
- dirty(bp);
- idesc->id_loc += DIRBLKSIZ;
- idesc->id_filesize -= DIRBLKSIZ;
- return (dp);
+ dp = (struct direct *)(bp->b_un.b_buf + idesc->id_loc);
+ /*
+ * Only need to check current entry if it is the first in the
+ * the block, as later entries will have been checked in the
+ * previous call to this function.
+ */
+ if (idesc->id_loc % DIRBLKSIZ != 0 || dircheck(idesc, bp, dp) != 0) {
+ /*
+ * Current entry is good, update to point at next.
+ */
+ idesc->id_loc += dp->d_reclen;
+ idesc->id_filesize -= dp->d_reclen;
+ /*
+ * If at end of directory block, just return this entry.
+ */
+ if (idesc->id_filesize <= 0 || idesc->id_loc >= blksiz ||
+ idesc->id_loc % DIRBLKSIZ == 0)
+ return (dp);
+ /*
+ * If the next entry good, return this entry.
+ */
+ ndp = (struct direct *)(bp->b_un.b_buf + idesc->id_loc);
+ if (dircheck(idesc, bp, ndp) != 0)
+ return (dp);
+ /*
+ * The next entry is bad, so subsume it and the remainder
+ * of this directory block into this entry.
+ */
+ subsume_ndp = 1;
}
-dpok:
- if (idesc->id_filesize <= 0 || idesc->id_loc >= blksiz)
- return NULL;
- dploc = idesc->id_loc;
- dp = (struct direct *)(bp->b_un.b_buf + dploc);
- idesc->id_loc += dp->d_reclen;
- idesc->id_filesize -= dp->d_reclen;
- if ((idesc->id_loc % DIRBLKSIZ) == 0)
- return (dp);
- ndp = (struct direct *)(bp->b_un.b_buf + idesc->id_loc);
- if (idesc->id_loc < blksiz && idesc->id_filesize > 0) {
- if ((dc = dircheck(idesc, ndp)) == 0) {
- size = DIRBLKSIZ - (idesc->id_loc % DIRBLKSIZ);
- idesc->id_loc += size;
- idesc->id_filesize -= size;
- if (idesc->id_fix == IGNORE)
- return (0);
- fix = dofix(idesc, "DIRECTORY CORRUPTED");
- bp = getdirblk(idesc->id_blkno, blksiz);
- dp = (struct direct *)(bp->b_un.b_buf + dploc);
- dp->d_reclen += size;
- if (fix)
- dirty(bp);
- } else if (dc == 2) {
- /*
- * dircheck() cleared unused directory space.
- * Mark the buffer as dirty to write it out.
- */
- dirty(bp);
- }
+ /*
+ * Current or next entry is bad. Zap current entry or
+ * subsume next entry into current entry as appropriate.
+ */
+ size = DIRBLKSIZ - (idesc->id_loc % DIRBLKSIZ);
+ idesc->id_loc += size;
+ idesc->id_filesize -= size;
+ if (idesc->id_fix == IGNORE)
+ return (NULL);
+ if (subsume_ndp) {
+ memset(ndp, 0, size);
+ dp->d_reclen += size;
+ } else {
+ memset(dp, 0, size);
+ dp->d_reclen = size;
}
+ if (dofix(idesc, "DIRECTORY CORRUPTED"))
+ dirty(bp);
return (dp);
}
@@ -219,65 +211,80 @@ dpok:
* This is a superset of the checks made in the kernel.
* Also optionally clears padding and unused directory space.
*
- * Returns 0 if the entry is bad, 1 if the entry is good and no changes
- * were made, and 2 if the entry is good but modified to clear out padding
- * and unused space and needs to be written back to disk.
+ * Returns 0 if the entry is bad, 1 if the entry is good.
*/
static int
-dircheck(struct inodesc *idesc, struct direct *dp)
+dircheck(struct inodesc *idesc, struct bufarea *bp, struct direct *dp)
{
size_t size;
char *cp;
- u_char type;
u_int8_t namlen;
int spaceleft, modified, unused;
- modified = 0;
spaceleft = DIRBLKSIZ - (idesc->id_loc % DIRBLKSIZ);
+ size = DIRSIZ(0, dp);
if (dp->d_reclen == 0 ||
dp->d_reclen > spaceleft ||
+ dp->d_reclen < size ||
+ idesc->id_filesize < size ||
(dp->d_reclen & (DIR_ROUNDUP - 1)) != 0)
goto bad;
+ modified = 0;
if (dp->d_ino == 0) {
+ if (!zflag || fswritefd < 0)
+ return (1);
/*
- * Special case of an unused directory entry. Normally
- * the kernel would coalesce unused space with the previous
- * entry by extending its d_reclen, but there are situations
- * (e.g. fsck) where that doesn't occur.
- * If we're clearing out directory cruft (-z flag), then make
- * sure this entry gets fully cleared as well.
+ * Special case of an unused directory entry. Normally only
+ * occurs at the beginning of a directory block when the block
+ * contains no entries. Other than the first entry in a
+ * directory block, the kernel coalesces unused space with
+ * the previous entry by extending its d_reclen. However,
+ * when cleaning up a directory, fsck may set d_ino to zero
+ * in the middle of a directory block. If we're clearing out
+ * directory cruft (-z flag), then make sure that all directory
+ * space in entries with d_ino == 0 gets fully cleared.
*/
- if (zflag && fswritefd >= 0) {
- if (dp->d_type != 0) {
- dp->d_type = 0;
+ if (dp->d_type != 0) {
+ dp->d_type = 0;
+ modified = 1;
+ }
+ if (dp->d_namlen != 0) {
+ dp->d_namlen = 0;
+ modified = 1;
+ }
+ unused = dp->d_reclen - __offsetof(struct direct, d_name);
+ for (cp = dp->d_name; unused > 0; unused--, cp++) {
+ if (*cp != '\0') {
+ *cp = '\0';
modified = 1;
}
- if (dp->d_namlen != 0) {
- dp->d_namlen = 0;
- modified = 1;
- }
- if (dp->d_name[0] != '\0') {
- dp->d_name[0] = '\0';
- modified = 1;
- }
}
- goto good;
+ if (modified)
+ dirty(bp);
+ return (1);
}
- size = DIRSIZ(0, dp);
+ /*
+ * The d_type field should not be tested here. A bad type is an error
+ * in the entry itself but is not a corruption of the directory
+ * structure itself. So blowing away all the remaining entries in the
+ * directory block is inappropriate. Rather the type error should be
+ * checked in pass1 and fixed there.
+ *
+ * The name validation should also be done in pass1 although the
+ * check to see if the name is longer than fits in the space
+ * allocated for it (i.e., the *cp != '\0' fails after exiting the
+ * loop below) then it really is a structural error that requires
+ * the stronger action taken here.
+ */
namlen = dp->d_namlen;
- type = dp->d_type;
- if (dp->d_reclen < size ||
- idesc->id_filesize < size ||
- namlen == 0 ||
- type > 15)
+ if (namlen == 0 || dp->d_type > 15)
goto bad;
- for (cp = dp->d_name, size = 0; size < namlen; size++)
- if (*cp == '\0' || (*cp++ == '/'))
+ for (cp = dp->d_name, size = 0; size < namlen; size++) {
+ if (*cp == '\0' || *cp++ == '/')
goto bad;
+ }
if (*cp != '\0')
goto bad;
-
-good:
if (zflag && fswritefd >= 0) {
/*
* Clear unused directory entry space, including the d_name
@@ -300,11 +307,9 @@ good:
}
}
- if (modified) {
- return 2;
- }
+ if (modified)
+ dirty(bp);
}
-
return (1);
bad:
More information about the svn-src-all
mailing list