Re: [PATCH] btrfs-progs: print-tree: fix chunk/block group flags output

On 2021/10/12 18:38, Nikolay Borisov wrote:

On 12.10.21 г. 13:35, Nikolay Borisov wrote:

quoted

<snip>

quoted

Signed-off-by: Qu Wenruo <redacted>
---
  kernel-shared/print-tree.c | 47 +++++++++++++++++++++++---------------
  1 file changed, 29 insertions(+), 18 deletions(-)

diff --git a/kernel-shared/print-tree.c b/kernel-shared/print-tree.c
index 67b654e6d2d5..39655590272e 100644
--- a/kernel-shared/print-tree.c
+++ b/kernel-shared/print-tree.c

@@ -159,40 +159,51 @@ static void print_inode_ref_item(struct extent_buffer *eb, u32 size,
  	}
  }

-/* Caller should ensure sizeof(*ret)>=21 "DATA|METADATA|RAID10" */
+/* The minimal length for the string buffer of block group/chunk flags */
+#define BG_FLAG_STRING_LEN	64
+
  static void bg_flags_to_str(u64 flags, char *ret)
  {
  	int empty = 1;
+	char profile[BG_FLAG_STRING_LEN] = {};
  	const char *name;

+	ret[0] = '\0';
  	if (flags & BTRFS_BLOCK_GROUP_DATA) {
  		empty = 0;
-		strcpy(ret, "DATA");
+		strncpy(ret, "DATA", BG_FLAG_STRING_LEN);

I find using strncpy rather odd, it guarantees it will copy num
characters, and if source is smaller than dest, it will overwrite the
rest with 0. So what happens is you are copying 4 chars here, and
writing 60 zeros. Frankly I think it's better to use >>
snprintf(ret, BG_FLAG_STRING_LEN, "DATA");

Well, you just told me a new fact, strncpy() would set the the rest bytes.

I thought it would just add the terminal '\0' if it's not reaching the
size limit.

But you're right, strncpy() would reset the padding bytes to zero.

quoted

quoted

  	}
  	if (flags & BTRFS_BLOCK_GROUP_METADATA) {
  		if (!empty)
-			strcat(ret, "|");
-		strcat(ret, "METADATA");
+			strncat(ret, "|", BG_FLAG_STRING_LEN);
+		strncat(ret, "METADATA", BG_FLAG_STRING_LEN);
  	}
  	if (flags & BTRFS_BLOCK_GROUP_SYSTEM) {
  		if (!empty)
-			strcat(ret, "|");
-		strcat(ret, "SYSTEM");
+			strncat(ret, "|", BG_FLAG_STRING_LEN);
+		strncat(ret, "SYSTEM", BG_FLAG_STRING_LEN);
  	}
-	strcat(ret, "|");
  	name = btrfs_bg_type_to_raid_name(flags);
  	if (!name) {
-		strcat(ret, "UNKNOWN");
+		snprintf(profile, BG_FLAG_STRING_LEN, "UNKNOWN.0x%llx",
+			 flags & BTRFS_BLOCK_GROUP_PROFILE_MASK);
  	} else {
-		char buf[32];
-		char *tmp = buf;
+		int i;

-		strcpy(buf, name);
-		while (*tmp) {
-			*tmp = toupper(*tmp);
-			tmp++;
-		}
-		strcpy(ret, buf);
+		/*
+		 * Special handing for SINGLE profile, we don't output "SINGLE"
+		 * for SINGLE profile, since there is no such bit for it.
+		 * Thus here we only fill @profile if it's not single.
+		 */
+		if (strncmp(name, "single", strlen("single")) != 0)
+			strncpy(profile, name, BG_FLAG_STRING_LEN);
+
+		for (i = 0; i < BG_FLAG_STRING_LEN && profile[i]; i++)

nit: It's guaranteed that the profile is shorted than
BG_FLAG_STRING_LEN, then this check can simply be profile[i] without the
i/BG_FLAG_STRING_LEN constant comparison.

I don't want to do any assumption here.

In fact I originally wanted to choose 32 as BG_FLAG_STRING_LEN, but it's
not safe already.

Considering the following output:
"DATA|METADATA|UNKNOWN.0xffffffff00000000"

Which is already 40 chars.

Since we're already doing all the strn*() calls just to avoid such
pitfalls, I tend to be extra safe here, thus I hope to keep the extra
check against BG_FLAG_STRING_LEN.

quoted

quoted

+			profile[i] = toupper(profile[i]);
+	}
+	if (profile[0]) {

Actually profile[0] here is guaranteed to be nonul - it's either
UNKNOWN... or whatever btrfs_bg_type_to_raid_name returned. So you can
simply use the strncat functions without needing the if.

You forgot SINGLE type.

In that case, profile[0] can be "\0".

quoted

quoted

+		strncat(ret, "|", BG_FLAG_STRING_LEN);
+		strncat(ret, profile, BG_FLAG_STRING_LEN);
  	}

This if can really be put in the above 'else' branch and eliminate the
check altogether.

Then these lines needs to be copied to two branches:

- UNKNOWN branch
- Non-SINGLE branch

Thus it's better to kept inside the if (profile[0]) branch, as it covers
two cases.

Thanks,
Qu

quoted

quoted

  }

<snip>