[PATCH 0/2] btrfs-progs: convert: fix the rollback filename output

[PATCH 0/2] btrfs-progs: convert: fix the rollback filename output

[Qu Wenruo]

When testing my newer version of subvolume creation cleanup (which
removes btrfs_mksubvol(), utilized by convert), I found out that
"btrfs-convert -r" output is always corrupted.

It turns out to be a small string termination problem.

The first patch fixes it, then a new test case for it.

Qu Wenruo (2):
  btrfs-progs: convert: fix the filename output when rolling back
  btrfs-progs: convert-tests: new test case to verify the rollback
    output

 convert/main.c                                |  2 +-
 .../convert-tests/026-rollback-output/test.sh | 26 +++++++++++++++++++
 2 files changed, 27 insertions(+), 1 deletion(-)
 create mode 100755 tests/convert-tests/026-rollback-output/test.sh

--
2.45.2

[PATCH 1/2] btrfs-progs: convert: fix the filename output when rolling back

[Qu Wenruo]

[BUG]
When rolling back a converted btrfs, the filename output is corrupted:

 $ btrfs-convert -r  ~/test.img
 btrfs-convert from btrfs-progs v6.9.2

 Open filesystem for rollback:
   Label:
   UUID:            df54baf3-c91e-4956-96f9-99413a857576
   Restoring from:  ext2_saved0ƨy/image
                              ^^^ Corruption
 Rollback succeeded

[CAUSE]
The error is in how we handle the filename.
In btrfs all our strings are not '\0' terminated, but with explicit
length.

But in C, most strings are '\0' terminated, so after reading a filename
from btrfs, we need to manually terminate the string.

However the code adding the terminating '\0' looks like this:

	/* Get the filename length. */
	name_len = btrfs_root_ref_name_len(path.nodes[0], root_ref_item);

	/*
	 * This should not happen, but as an extra handling for possible
	 * corrupted btrfs.
	 */
	if (name_len > sizeof(dir_name))
		name_len = sizeof(dir_name) - 1;
	/* Got the real filename into our buffer. */
 	read_extent_buffer(path.nodes[0], dir_name, (unsigned long)(root_ref_item + 1), name_len);

	/* Terminate the string. */
	dir_name[sizeof(dir_name) - 1] = 0;

The problem is, the final termination is totally wrong, it always make
the last buffer char '\0', not using the @name_len we read before.

[FIX]
Use @name_len to terminate the string, as we have already updated it to
handle buffer overflow, it can handle both the regular and corrupted
case.

Fixes: dc29a5c51d63 ("btrfs-progs: convert: update default output")
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 convert/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/convert/main.c b/convert/main.c
index 8e73aa25b1da..dfb9f44f6f75 100644
--- a/convert/main.c
+++ b/convert/main.c
@@ -1720,7 +1720,7 @@ static int do_rollback(const char *devname)
 	if (name_len > sizeof(dir_name))
 		name_len = sizeof(dir_name) - 1;
 	read_extent_buffer(path.nodes[0], dir_name, (unsigned long)(root_ref_item + 1), name_len);
-	dir_name[sizeof(dir_name) - 1] = 0;
+	dir_name[name_len] = 0;
 
 	printf("  Restoring from:  %s/%s\n", dir_name, image_name);
 
-- 
2.45.2

[PATCH 2/2] btrfs-progs: convert-tests: new test case to verify the rollback output

[Qu Wenruo]

The new new test case is to make sure the rollback output for a fixed
content converted fs contains the string "ext2_saved/image".

As we have a bug in the past where after the string "ext2_saved", we can
have some unterminated garbage.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 .../convert-tests/026-rollback-output/test.sh | 26 +++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100755 tests/convert-tests/026-rollback-output/test.sh

diff --git a/tests/convert-tests/026-rollback-output/test.sh b/tests/convert-tests/026-rollback-output/test.sh
new file mode 100755
index 000000000000..ed3d14c1aa5e
--- /dev/null
+++ b/tests/convert-tests/026-rollback-output/test.sh
@@ -0,0 +1,26 @@
+#!/bin/bash
+# Make sure "btrfs-convert -r" is outputting the correct filename
+
+source "$TEST_TOP/common" || exit
+source "$TEST_TOP/common.convert" || exit
+
+setup_root_helper
+prepare_test_dev
+
+check_global_prereq mkfs.ext4
+check_prereq btrfs-convert
+check_prereq btrfs
+
+convert_test_prep_fs ext4 mke2fs -t ext4 -b 4096
+run_check_umount_test_dev
+convert_test_do_convert
+
+tmp=$(mktemp --tmpdir btrfs-progs-convert-rollback.XXXXXX)
+# Rollback and save the output.
+run_check_stdout "$TOP/btrfs-convert" --rollback "$TEST_DEV" >> "$tmp"
+
+if ! grep -q "ext2_saved/image" "$tmp"; then
+	rm -f -- "$tmp"
+	_fail "rollback filename output is corruptedd"
+fi
+rm -f -- "$tmp"
-- 
2.45.2