Re: [PATCH v2] btrfs-progs: cmds: Add subcommand that dumps file extents

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2021/7/14 上午12:54, Sidong Yang wrote:
> On Mon, Jul 12, 2021 at 06:52:28AM +0000, Johannes Thumshirn wrote:
>
> Hi, Thanks for review!
>
>> On 11/07/2021 18:10, Sidong Yang wrote:
>>> +static void compress_type_to_str(u8 compress_type, char *ret)
>>> +{
>>> +	switch (compress_type) {
>>> +	case BTRFS_COMPRESS_NONE:
>>> +		strcpy(ret, "none");
>>> +		break;
>>> +	case BTRFS_COMPRESS_ZLIB:
>>> +		strcpy(ret, "zlib");
>>> +		break;
>>> +	case BTRFS_COMPRESS_LZO:
>>> +		strcpy(ret, "lzo");
>>> +		break;
>>> +	case BTRFS_COMPRESS_ZSTD:
>>> +		strcpy(ret, "zstd");
>>> +		break;
>>> +	default:
>>> +		sprintf(ret, "UNKNOWN.%d", compress_type);
>>> +	}
>>> +}
>>
>> [....]
>>> +	char compress_str[16];
>>
>> [...]
>>
>>> +					compress_type_to_str(extent_item->compression, compress_str);
>>
>> While this looks safe at a first glance, can we change this to:
>>
>> #define COMPRESS_STR_LEN 5
>>
>> [...]
>>
>> switch (compress_type) {
>> case BTRFS_COMPRESS_NONE:
>> 	strncpy(ret, "none", COMPRESS_STR_LEN);
>>
>> [...]
>>
>> char compress_str[COMPRESS_STR_LEN];
>>
>> One day someone will factor out 'compress_type_to_str()' and make it public, read user
>> supplied input and then it's a disaster waiting to happen.
>
> This can be happen when @ret is too short? If it so, I think it also has
> problem when @ret is shorter than COMPRESS_STR_LEN. I wonder that I
> understood this problem you said.

I guess Johannes means that, if we support more and more new compression
algos, it can go beyond your original 16 bytes length for the
compression algo name.

While using strncpy, it will never go beyond COMPRESS_STR_LEN forever.
(Although it will truncating the output, but that would be easier to
spot the problem and then enlarge the macro)

Thanks,
Qu

>
> Thanks,
> Sidong
>
>>
>> Thanks,
>> 	Johannes