Re: tune2fs: prevent crash if error in journal recovery

["Theodore Ts'o" <tytso@xxxxxxx>]

On Thu, Oct 05, 2017 at 08:25:50AM +0300, eru@xxxxxxxx wrote:
> Prevent coredump if tunefs calls ext2fs_run_ext3_journal(), and this 
> then
> fails for some reason while calling ext2fs_open2(). Such failure makes
> the fs pointer NULL.
> 
> Signed-off-by: Erkki Ruohtula <eru@xxxxxxxx>

Hi,

Thanks for reporting this issue and sending a patch.  It turns out
there were more problems hiding ext2fs_run_ext3_journal() than just
the one you pointed out, and so the clean up was more extensive.

    	    	    	     	- Ted

commit 6ae16a6814f47c96c261519380df7a7820c8f468
Author: Theodore Ts'o <tytso@xxxxxxx>
Date:   Sun Oct 15 00:22:20 2017 -0400

    misc: clean up error handling for ext2fs_run_ext3_journal()
    
    The ext2fs_run_ext3_journal() function is in debugfs/journal.c, and in
    some error conditions cases may close the passed-in file system handle.
    
    Clean up the both the function so that it reliably clears the file
    system handle if it has been freed, and its callers so that they do
    not crash by dereferencing a null pointer if it has been freed.
    
    Reported-by: Erkki Ruohtula <eru@xxxxxxxx>
    Signed-off-by: Theodore Ts'o <tytso@xxxxxxx>

diff --git a/debugfs/journal.c b/debugfs/journal.c
index 9997c7c7c..56a68be52 100644
--- a/debugfs/journal.c
+++ b/debugfs/journal.c
@@ -793,14 +793,14 @@ errcode_t ext2fs_run_ext3_journal(ext2_filsys *fsp)
 		kbytes_written = stats->bytes_written >> 10;
 
 	ext2fs_mmp_stop(fs);
-	fsname = strdup(fs->device_name);
+	fsname = fs->device_name;
+	fs->device_name = NULL;
 	fsflags = fs->flags;
 	fsblocksize = fs->blocksize;
 	ext2fs_free(fs);
-	retval = ext2fs_open(fsname, fsflags,
-			     0, fsblocksize, io_ptr,
-			     fsp);
-	free(fsname);
+	*fsp = NULL;
+	retval = ext2fs_open(fsname, fsflags, 0, fsblocksize, io_ptr, fsp);
+	ext2fs_free_mem(&fsname);
 	if (retval)
 		return retval;
 
diff --git a/misc/fuse2fs.c b/misc/fuse2fs.c
index b5897685c..f14cefbad 100644
--- a/misc/fuse2fs.c
+++ b/misc/fuse2fs.c
@@ -3759,7 +3759,7 @@ int main(int argc, char *argv[])
 		fctx.err_fp = fopen(logfile, "a");
 		if (!fctx.err_fp) {
 			perror(logfile);
-			goto out_nofs;
+			goto out;
 		}
 	} else
 		fctx.err_fp = stderr;
@@ -3780,7 +3780,7 @@ int main(int argc, char *argv[])
 	if (err) {
 		printf(_("%s: %s.\n"), fctx.device, error_message(err));
 		printf(_("Please run e2fsck -fy %s.\n"), fctx.device);
-		goto out_nofs;
+		goto out;
 	}
 	fctx.fs = global_fs;
 	global_fs->priv_data = &fctx;
@@ -3869,12 +3869,12 @@ int main(int argc, char *argv[])
 
 	ret = 0;
 out:
-	err = ext2fs_close(global_fs);
-	if (err)
-		com_err(argv[0], err, "while closing fs");
-	global_fs = NULL;
-out_nofs:
-
+	if (global_fs) {
+		err = ext2fs_close(global_fs);
+		if (err)
+			com_err(argv[0], err, "while closing fs");
+		global_fs = NULL;
+	}
 	return ret;
 }
 
diff --git a/misc/tune2fs.c b/misc/tune2fs.c
index 3e7ca23e1..db06003e0 100644
--- a/misc/tune2fs.c
+++ b/misc/tune2fs.c
@@ -465,9 +465,6 @@ static void request_fsck_afterwards(ext2_filsys fs)
 
 static void convert_64bit(ext2_filsys fs, int direction)
 {
-	if (!direction)
-		return;
-
 	/*
 	 * Is resize2fs going to demand a fsck run? Might as well tell the
 	 * user now.
@@ -3238,7 +3235,9 @@ _("Warning: The journal is dirty. You may wish to replay the journal like:\n\n"
 		if (err) {
 			com_err("tune2fs", err, "while recovering journal.\n");
 			printf(_("Please run e2fsck -fy %s.\n"), argv[1]);
-			goto closefs;
+			if (fs)
+				ext2fs_close_free(&fs);
+			exit(1);
 		}
 		ext2fs_clear_feature_journal_needs_recovery(fs->super);
 		ext2fs_mark_super_dirty(fs);
@@ -3256,6 +3255,7 @@ closefs:
 #endif
 	}
 
-	convert_64bit(fs, feature_64bit);
+	if (feature_64bit)
+		convert_64bit(fs, feature_64bit);
 	return (ext2fs_close_free(&fs) ? 1 : 0);
 }