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

[Sidong Yang <realwakka@gmail.com>]

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.

Thanks,
Sidong

> 
> Thanks,
> 	Johannes