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

[Sidong Yang <realwakka@gmail.com>]

On Wed, Jul 14, 2021 at 06:16:08AM +0800, Qu Wenruo wrote:
> 
> 
> 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)
Okay, I understood. I'll do this in next version.

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