bin/47681: Suspect code in fsck_ext2fs

[Antoine.Leca.1%gmail.com@localhost]

>Number:         47681
>Category:       bin
>Synopsis:       Suspect code in fsck_ext2fs
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    bin-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Fri Mar 22 08:05:00 +0000 2013
>Originator:     Antoine Leca
>Release:        NetBSD 6.0
>Organization:
>Environment:
System: NetBSD netbsd6.local 6.0 NetBSD 6.0 (GENERIC) amd64
>Description:
I found what I suspect to be a problem in fsck_ext2fs/dir.c. My understanding 
is that in that function dircheck(), returning (1) means all_is_good, while (0) 
indicates some problem; also, I understand a non-zero e2d_type in some entry 
while either using Rev.0 or with the INCOMPAT_FTYPE unset, is NOT a normal 
condition.

Unfortunately due my own bugs (in foreign code) I am not able to confirm 
whether it needs to be fixed or not.

While here, I realized that the actual value for e2d_type was not checked in 
the case of Rev.1 FS, so I designed another patch (2nd) to add that check. No 
doubt it can be better written!

Perhaps related is that, when the function returns(0) while there are dirty 
values within the directory entries, then fsck stops on that directory and 
proposes to "Salvage" the directory, which I read to be a pretty drastic 
operation, dropping all the entries after the faulty one if I understand 
correctly (I am all of a newbie to FFS and derivatives, so do not hesitate to 
correct me where I am wrong.)
>How-To-Repeat:
Take some (dispensable) ext2fs file system.
Change the e2d_type byte of a directory entry.
Then run fsck_ext2fs -f against that file system.
>Fix:
Obvious fix:

diff --git a/sbin/fsck_ext2fs/dir.c b/sbin/fsck_ext2fs/dir.c
index 8036f52..66d36ec 100644
--- a/sbin/fsck_ext2fs/dir.c
+++ b/sbin/fsck_ext2fs/dir.c
@@ -274,7 +274,7 @@ dircheck(struct inodesc *idesc, struct ext2fs_direct *dp)
        if (sblock.e2fs.e2fs_rev < E2FS_REV1 ||
            (sblock.e2fs.e2fs_features_incompat & EXT2F_INCOMPAT_FTYPE) == 0)
                if (dp->e2d_type != 0)
-                       return (1);
+                       return (0);
        size = EXT2FS_DIRSIZ(dp->e2d_namlen);
        if (reclen < size ||
            idesc->id_filesize < size /* || 



Further improvement of the utility:
diff --git a/sbin/fsck_ext2fs/dir.c b/sbin/fsck_ext2fs/dir.c
index 66d36ec..f6327eb 100644
--- a/sbin/fsck_ext2fs/dir.c
+++ b/sbin/fsck_ext2fs/dir.c
@@ -272,9 +272,13 @@ dircheck(struct inodesc *idesc, struct ext2fs_direct *dp)
        if (dp->e2d_ino == 0)
                return (1);
        if (sblock.e2fs.e2fs_rev < E2FS_REV1 ||
-           (sblock.e2fs.e2fs_features_incompat & EXT2F_INCOMPAT_FTYPE) == 0)
+           (sblock.e2fs.e2fs_features_incompat & EXT2F_INCOMPAT_FTYPE) == 0) {
                if (dp->e2d_type != 0)
                        return (0);
+       } else {
+               if (dp->e2d_type >= EXT2_FT_MAX)
+                       return (0);
+       }
        size = EXT2FS_DIRSIZ(dp->e2d_namlen);
        if (reclen < size ||
            idesc->id_filesize < size /* ||