tune2fs crash in error situation

[eru@xxxxxxxx]

tune2fs seems to coredump if the file system is not clean, so that it 
goes
to the branch calling ext2fs_run_ext3_journal(), and this then fails for
some reason while calling ext2fs_open2().

misc/tunefs.c:
..
        /* Recover the journal if possible. */
        if ((open_flag & EXT2_FLAG_RW) && !(mount_flags & 
EXT2_MF_MOUNTED) &&
            ext2fs_has_feature_journal_needs_recovery(fs->super)) {
                errcode_t err;

                printf(_("Recovering journal.\n"));
                err = ext2fs_run_ext3_journal(&fs);
                if (err) {
                        com_err("tune2fs", err, "while recovering 
journal.\n");
                        printf(_("Please run e2fsck -fy %s.\n"), 
argv[1]);
                        goto closefs;
                }
                ext2fs_clear_feature_journal_needs_recovery(fs->super);
                ext2fs_mark_super_dirty(fs);
        }

Failures in ext2fs_open2() zero the fs pointer, and the goto closefs
leads to code that will dereference fs:

closefs:
        if (rc) {
                ext2fs_mmp_stop(fs);
#ifndef BUILD_AS_LIB
                exit(1);
#endif
        }

        convert_64bit(fs, feature_64bit);
        return (ext2fs_close_free(&fs) ? 1 : 0);

calling ext2fs_close_free(&fs) leads to

lib/ext2fs/closefs.c:
..
errcode_t ext2fs_close2(ext2_filsys fs, int flags)
{
        errcode_t       retval;
        int             meta_blks;
        io_stats stats = 0;

        EXT2_CHECK_MAGIC(fs, EXT2_ET_MAGIC_EXT2FS_FILSYS);

and the dereference happens in that macro.

A crash because of this was observed when the device passed to tune2fs
was a /dev/disk/by-id/* link, and apparently udevd happened to 
manipulate
it while tune2fs was running. Or that at least is my theory for the
randomly observed error message:

 tune2fs: No such file or directory while recovering journal

that preceded the crash. This error situation could be reproduced
nondeterministically by a loop that calls tunefs with an unclean file
system (created by mounting an image to a VM, and the suddenly killing
the VM), and having the loop make the path invalid while tune2fs is 
running.

while true; do
  cp dirtyfs.img x.img
  ln -s /root/x.img /tmp/disk
  ./tune2fs -L somelabel /tmp/disk 2>>tunefs.error &
  sleep 0.1
  rm /tmp/disk
done

At least versions e2fsprogs-1.43.5 and e2fsprogs-1.43.6 are affected.
An obvious patch for the problem:

diff -ru e2fsprogs-1.43.6.orig/misc/tune2fs.c 
e2fsprogs-1.43.6/misc/tune2fs.c
--- e2fsprogs-1.43.6.orig/misc/tune2fs.c	2017-08-29 16:05:52.000000000 
+0300
+++ e2fsprogs-1.43.6/misc/tune2fs.c	2017-09-21 12:00:37.068873101 +0300
@@ -3256,6 +3256,9 @@
 #endif
 	}

-	convert_64bit(fs, feature_64bit);
-	return (ext2fs_close_free(&fs) ? 1 : 0);
+	if (fs) {
+		convert_64bit(fs, feature_64bit);
+		return (ext2fs_close_free(&fs) ? 1 : 0);
+	}
+	return 1;
 }


--
Erkki Ruohtula