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

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

On 2020/7/21 下午9:55, David Sterba wrote:
> 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.
>
> Ouch, I hope you don't mean that contributors should ignore the trickier
> parts of C language. Especially reviewers _have_ to bother about all
> sorts of subtle behaviour.
>
>> 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.
>
> ... and introduce bugs?
>
>> Thus casting both would definitely be right, without the need to refer
>> to the complex rule book, thus save the reviewer several minutes.
>
> The opposite, if you send me code that's not following known schemes or
> idiomatic schemes I'll be highly suspicious and looking for the reasons
> why it's that way and making sure it's correct costs way more time.
>
OK, then would you please remove one casting at merge time, or do I need
to resend?

Thanks,
Qu