linux-kernel - Re: [PATCH] btrfs: fix uninitialized warning in btrfs_log

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 17 May 2023 17:49:57 +0800
From:   Qu Wenruo <quwenruo.btrfs@....com>
To:     Stephen Zhang <starzhangzsd@...il.com>
Cc:     clm@...com, josef@...icpanda.com, dsterba@...e.com,
        linux-btrfs@...r.kernel.org, linux-kernel@...r.kernel.org,
        zhangshida@...inos.cn, k2ci <kernel-bot@...inos.cn>
Subject: Re: [PATCH] btrfs: fix uninitialized warning in btrfs_log_inode



On 2023/5/17 17:07, Stephen Zhang wrote:
> Qu Wenruo <quwenruo.btrfs@....com> 于2023年5月17日周三 15:47写道：
>>
>>
>>
>> On 2023/5/16 09:34, zhangshida wrote:
>>> From: Shida Zhang <zhangshida@...inos.cn>
>>>
>>> From: Shida Zhang <zhangshida@...inos.cn>
>>>
>>> This fixes the following warning reported by gcc 10 under x86_64:
>>
>> Full gcc version please.
>
> it's "gcc (Debian 10.2.1-6) 10.2.1 20210110".

 From GCC 10 release notes:

 > GCC 10.4
 >     June 28, 2022 (changes, documentation)

Please upgrade to latest 10.x, at least Debian should already have gcc
10.4.x in their repos.

>
>> Especially you need to check if your gcc10 is the latest release.
>>
>> If newer gcc (12.2.1) tested without such error, it may very possible to
>> be a false alert.
>>
>> And in fact it is.
>>
>> @first_dir_index would only be assigned to @last_range_start if
>> last_range_end != 0.
>>
>> Thus the loop must have to be executed once, and @last_range_start won't
>> be zero.
>>
>
> Yup, I know it's a false positive. What I don't know is the criterion
> that decides whether it is a good patch.
> That is,
> it doesn't look so good because it is a false alert and the latest gcc
> can get rid of such warnings, based on what you said( if I understand
> correctly).
> Or,
> It looks okay because the patch can make some older gcc get a cleaner
> build and do no harm to the original code logic.

To me, we want every variable not to be initialized unless necessary, so
compiler can do us a favor to detect unexpected corner cases.

Just unconditionally silent it is not a good way to go, not to mention
the initial value may even be wrong for other cases (not this particular
case).


If you want to fix the situation (other than upgrading your tool chain),
please do it in a way that is also improving the code.

For this particular case, I don't have a particular good suggestion.

But I may introduce a bool to indicate if we have hit any ranges before,
like @has_last_range, then assign no initial value to  @last_range_end.

With this, we bind everything requiring the loop to be executed at least
once to a single bool variable, then maybe older compiler is also able
to detect it's a false alert.

Thanks,
Qu

> In fact, I've seen Linus complaining about the warning generated by
> some gcc version in another thread.
>
> https://lore.kernel.org/linux-xfs/168384265493.22863.2683852857659893778.pr-tracker-bot@kernel.org/T/#t
>
> so it kinda make me feel confused :<
>
> Nonetheless, I appreciate your review.
>
> Thanks,
> Shida
>
>> Please do check your environment (especially your gcc version and
>> backports), before sending such trivial patches.
>> Under most cases, it helps nobody.
>>
>> Thanks,
>> Qu
>>
>>>
>>> ../fs/btrfs/tree-log.c: In function ‘btrfs_log_inode’:
>>> ../fs/btrfs/tree-log.c:6211:9: error: ‘last_range_start’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>>    6211 |   ret = insert_dir_log_key(trans, log, path, key.objectid,
>>>         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>    6212 |       first_dir_index, last_dir_index);
>>>         |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> ../fs/btrfs/tree-log.c:6161:6: note: ‘last_range_start’ was declared here
>>>    6161 |  u64 last_range_start;
>>>         |      ^~~~~~~~~~~~~~~~
>>>
>>> Reported-by: k2ci <kernel-bot@...inos.cn>
>>> Signed-off-by: Shida Zhang <zhangshida@...inos.cn>
>>> ---
>>>    fs/btrfs/tree-log.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
>>> index 9b212e8c70cc..d2755d5e338b 100644
>>> --- a/fs/btrfs/tree-log.c
>>> +++ b/fs/btrfs/tree-log.c
>>> @@ -6158,7 +6158,7 @@ static int log_delayed_deletions_incremental(struct btrfs_trans_handle *trans,
>>>    {
>>>        struct btrfs_root *log = inode->root->log_root;
>>>        const struct btrfs_delayed_item *curr;
>>> -     u64 last_range_start;
>>> +     u64 last_range_start = 0;
>>>        u64 last_range_end = 0;
>>>        struct btrfs_key key;
>>>