[PATCH] btrfs: avoid using fixed char array size for tree names

[PATCH] btrfs: avoid using fixed char array size for tree names

[Qu Wenruo]

[BUG]
There is a bug report that using the latest trunk GCC, btrfs would cause
unterminated-string-initialization warning:

  linux-6.6/fs/btrfs/print-tree.c:29:49: error: initializer-string for array of ‘char’ is too long [-Werror=unterminated-string-initialization]
   29 |         { BTRFS_BLOCK_GROUP_TREE_OBJECTID,      "BLOCK_GROUP_TREE"      },
      |
      ^~~~~~~~~~~~~~~~~~

[CAUSE]
To print tree names we have an array of root_name_map structure, which
uses "char name[16];" to store the name string of a tree.

But the following trees have names exactly at 16 chars length:
- "BLOCK_GROUP_TREE"
- "RAID_STRIPE_TREE"

This means we will have no space for the terminating '\0', and can lead
to unexpected access when printing the name.

[FIX]
Instead of "char name[16];" use "const char *" instead.

Since the name strings are all read-only data, and are all NULL
terminated by default, there is not much need to bother the length at
all.

Reported-by: Sam James <sam@gentoo.org>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/print-tree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/print-tree.c b/fs/btrfs/print-tree.c
index 32dcea662da3..fc821aa446f0 100644
--- a/fs/btrfs/print-tree.c
+++ b/fs/btrfs/print-tree.c
@@ -14,7 +14,7 @@
 
 struct root_name_map {
 	u64 id;
-	char name[16];
+	const char *name;
 };
 
 static const struct root_name_map root_map[] = {
-- 
2.45.2

Re: [PATCH] btrfs: avoid using fixed char array size for tree names

[Sam James]

[-- Attachment #1: Type: text/plain, Size: 1710 bytes --]

Qu Wenruo <wqu@suse.com> writes:

> [BUG]
> There is a bug report that using the latest trunk GCC, btrfs would cause
> unterminated-string-initialization warning:
>
>   linux-6.6/fs/btrfs/print-tree.c:29:49: error: initializer-string for array of ‘char’ is too long [-Werror=unterminated-string-initialization]
>    29 |         { BTRFS_BLOCK_GROUP_TREE_OBJECTID,      "BLOCK_GROUP_TREE"      },
>       |
>       ^~~~~~~~~~~~~~~~~~
>
> [CAUSE]
> To print tree names we have an array of root_name_map structure, which
> uses "char name[16];" to store the name string of a tree.
>
> But the following trees have names exactly at 16 chars length:
> - "BLOCK_GROUP_TREE"
> - "RAID_STRIPE_TREE"
>
> This means we will have no space for the terminating '\0', and can lead
> to unexpected access when printing the name.
>
> [FIX]
> Instead of "char name[16];" use "const char *" instead.
>
> Since the name strings are all read-only data, and are all NULL
> terminated by default, there is not much need to bother the length at
> all.
>
> Reported-by: Sam James <sam@gentoo.org>
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Thank you for the quick fix! I agree that this seems like the cleanest
change.

Tested-by: Sam James <sam@gentoo.org>

> ---
>  fs/btrfs/print-tree.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/print-tree.c b/fs/btrfs/print-tree.c
> index 32dcea662da3..fc821aa446f0 100644
> --- a/fs/btrfs/print-tree.c
> +++ b/fs/btrfs/print-tree.c
> @@ -14,7 +14,7 @@
>  
>  struct root_name_map {
>  	u64 id;
> -	char name[16];
> +	const char *name;
>  };
>  
>  static const struct root_name_map root_map[] = {

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 377 bytes --]

Re: [PATCH] btrfs: avoid using fixed char array size for tree names

[Josef Bacik]

On Fri, Jul 19, 2024 at 02:20:39PM +0930, Qu Wenruo wrote:
> [BUG]
> There is a bug report that using the latest trunk GCC, btrfs would cause
> unterminated-string-initialization warning:
> 
>   linux-6.6/fs/btrfs/print-tree.c:29:49: error: initializer-string for array of ‘char’ is too long [-Werror=unterminated-string-initialization]
>    29 |         { BTRFS_BLOCK_GROUP_TREE_OBJECTID,      "BLOCK_GROUP_TREE"      },
>       |
>       ^~~~~~~~~~~~~~~~~~
> 
> [CAUSE]
> To print tree names we have an array of root_name_map structure, which
> uses "char name[16];" to store the name string of a tree.
> 
> But the following trees have names exactly at 16 chars length:
> - "BLOCK_GROUP_TREE"
> - "RAID_STRIPE_TREE"
> 
> This means we will have no space for the terminating '\0', and can lead
> to unexpected access when printing the name.
> 
> [FIX]
> Instead of "char name[16];" use "const char *" instead.
> 
> Since the name strings are all read-only data, and are all NULL
> terminated by default, there is not much need to bother the length at
> all.
> 
> Reported-by: Sam James <sam@gentoo.org>
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH] btrfs: avoid using fixed char array size for tree names

[David Sterba]

On Fri, Jul 19, 2024 at 02:20:39PM +0930, Qu Wenruo wrote:
> [BUG]
> There is a bug report that using the latest trunk GCC, btrfs would cause
> unterminated-string-initialization warning:
> 
>   linux-6.6/fs/btrfs/print-tree.c:29:49: error: initializer-string for array of ‘char’ is too long [-Werror=unterminated-string-initialization]
>    29 |         { BTRFS_BLOCK_GROUP_TREE_OBJECTID,      "BLOCK_GROUP_TREE"      },
>       |
>       ^~~~~~~~~~~~~~~~~~
> 
> [CAUSE]
> To print tree names we have an array of root_name_map structure, which
> uses "char name[16];" to store the name string of a tree.
> 
> But the following trees have names exactly at 16 chars length:
> - "BLOCK_GROUP_TREE"
> - "RAID_STRIPE_TREE"
> 
> This means we will have no space for the terminating '\0', and can lead
> to unexpected access when printing the name.
> 
> [FIX]
> Instead of "char name[16];" use "const char *" instead.

Please use a fixed size string, this avoids the indirection of one
pointer and the actual strings. For static tables like this is a compact
way to store it. As the alignment is mandated by u64 the sizes would be
best in multipes of 8, so 'char name[24]'.

Re: [PATCH] btrfs: avoid using fixed char array size for tree names

[Qu Wenruo]

在 2024/7/20 09:23, David Sterba 写道:
> On Fri, Jul 19, 2024 at 02:20:39PM +0930, Qu Wenruo wrote:
>> [BUG]
>> There is a bug report that using the latest trunk GCC, btrfs would cause
>> unterminated-string-initialization warning:
>>
>>    linux-6.6/fs/btrfs/print-tree.c:29:49: error: initializer-string for array of ‘char’ is too long [-Werror=unterminated-string-initialization]
>>     29 |         { BTRFS_BLOCK_GROUP_TREE_OBJECTID,      "BLOCK_GROUP_TREE"      },
>>        |
>>        ^~~~~~~~~~~~~~~~~~
>>
>> [CAUSE]
>> To print tree names we have an array of root_name_map structure, which
>> uses "char name[16];" to store the name string of a tree.
>>
>> But the following trees have names exactly at 16 chars length:
>> - "BLOCK_GROUP_TREE"
>> - "RAID_STRIPE_TREE"
>>
>> This means we will have no space for the terminating '\0', and can lead
>> to unexpected access when printing the name.
>>
>> [FIX]
>> Instead of "char name[16];" use "const char *" instead.
>
> Please use a fixed size string, this avoids the indirection of one
> pointer and the actual strings.

I strongly doubt the necessary of avoiding indirection.

Just remember all of our error messages are some pointers to a ro data
section, and I see no reason why we need to bother the indirection or
whatever.

They are the cold path anyway, so is our tree names.

You can go char name[24], but without a proper macros checking the
string length, we're going to hit the same problem sooner or later.

So, I see no reason bothering extending the char size.
It's not extendable, nor safe.

> For static tables like this is a compact
> way to store it.

Nope, it's not compact at all, for shorter names we're just wasting
global ro data space.
The const char * solution is really using the minimal space.

Thanks,
Qu

> As the alignment is mandated by u64 the sizes would be
> best in multipes of 8, so 'char name[24]'.
>

Re: [PATCH] btrfs: avoid using fixed char array size for tree names

[Qu Wenruo]

在 2024/7/20 10:01, Qu Wenruo 写道:
>
>
> 在 2024/7/20 09:23, David Sterba 写道:
>> On Fri, Jul 19, 2024 at 02:20:39PM +0930, Qu Wenruo wrote:
>>> [BUG]
>>> There is a bug report that using the latest trunk GCC, btrfs would cause
>>> unterminated-string-initialization warning:
>>>
>>>    linux-6.6/fs/btrfs/print-tree.c:29:49: error: initializer-string
>>> for array of ‘char’ is too long
>>> [-Werror=unterminated-string-initialization]
>>>     29 |         { BTRFS_BLOCK_GROUP_TREE_OBJECTID,
>>> "BLOCK_GROUP_TREE"      },
>>>        |
>>>        ^~~~~~~~~~~~~~~~~~
>>>
>>> [CAUSE]
>>> To print tree names we have an array of root_name_map structure, which
>>> uses "char name[16];" to store the name string of a tree.
>>>
>>> But the following trees have names exactly at 16 chars length:
>>> - "BLOCK_GROUP_TREE"
>>> - "RAID_STRIPE_TREE"
>>>
>>> This means we will have no space for the terminating '\0', and can lead
>>> to unexpected access when printing the name.
>>>
>>> [FIX]
>>> Instead of "char name[16];" use "const char *" instead.
>>
>> Please use a fixed size string, this avoids the indirection of one
>> pointer and the actual strings.
>
> I strongly doubt the necessary of avoiding indirection.
>
> Just remember all of our error messages are some pointers to a ro data
> section, and I see no reason why we need to bother the indirection or
> whatever.
>
> They are the cold path anyway, so is our tree names.
>
> You can go char name[24], but without a proper macros checking the
> string length, we're going to hit the same problem sooner or later.
>
> So, I see no reason bothering extending the char size.
> It's not extendable, nor safe.
>
>> For static tables like this is a compact
>> way to store it.
>
> Nope, it's not compact at all, for shorter names we're just wasting
> global ro data space.
> The const char * solution is really using the minimal space.

If you really want to dig deeper, let me compare the bytes usages of
both methods on 64bit systems:

For name[24]:
13 * (8 + 24) = 416 bytes

For const char *name:

13 * (8 + 8) + (9 + 11 + 10 + 8 + 7 + 9 + 8 + 10 + 9 + 15 + 16 + 15 +
16) + 13 = 365 bytes

Now you can see which methods wastes more space.

Thanks,
Qu
>
> Thanks,
> Qu
>
>> As the alignment is mandated by u64 the sizes would be
>> best in multipes of 8, so 'char name[24]'.
>>
>