Re: [PATCH 1/2] btrfs-progs: convert: Prevent bit overflow for cctx->total_bytes

[Stefan Traby <stefan@hello-penguin.com>]

On Tue, Jul 21, 2020 at 06:29:31PM +0800, Qu Wenruo wrote:
> On 2020/7/21 下午5:58, David Sterba wrote:
> > On Tue, Jul 21, 2020 at 07:51:00AM +0800, Qu Wenruo wrote:
> >>
> >>
> >> On 2020/7/21 上午12:09, David Sterba wrote:
> >>> On Mon, Jul 20, 2020 at 08:51:08PM +0800, Qu Wenruo wrote:
> >>>> --- a/convert/source-ext2.c
> >>>> +++ b/convert/source-ext2.c
> >>>> @@ -87,7 +87,8 @@ static int ext2_open_fs(struct btrfs_convert_context *cctx, const char *name)
> >>>>  	cctx->fs_data = ext2_fs;
> >>>>  	cctx->blocksize = ext2_fs->blocksize;
> >>>>  	cctx->block_count = ext2_fs->super->s_blocks_count;
> >>>> -	cctx->total_bytes = ext2_fs->blocksize * ext2_fs->super->s_blocks_count;
> >>>> +	cctx->total_bytes = (u64)ext2_fs->blocksize *
> >>>> +			    (u64)ext2_fs->super->s_blocks_count;
> >>>
> >>> Do you need to cast both? Once one of the types is wide enough for the
> >>> result, there should be no loss.
> >>>
> >> I just want to be extra safe.
> > 
> > Typecasts in code raise questions why are they needed, 'to be extra'
> > safe is not a good reason. One typecast in multiplication/shifts is a
> > common pattern to widen the result but two look more like lack of
> > understanding of the integer promotion rules.
> > 
> 
> My point here is, I don't want the reviewers or new contributors to
> bother about the promotion rules at all.

Reviewers and contributors need to understand the rules of C.

> They only need to know that using blocksize and blocks_count directly to
> do multiply would lead to overflow.
> 
> Other details like whether the multiply follows the highest factor or
> the left operator or the right operator, shouldn't be the point and we
> don't really need to bother.

This is like suggesting to use brackets on every int-expression
like ((5*3)+7) because the precedere rules are too complex.
Do you think that writing ((5*3)+7) is "extra safe"?

> Thus casting both would definitely be right, without the need to refer
> to the complex rule book, thus save the reviewer several minutes.

I don't think so.

-- 

  ciao - 
    Stefan