linux-kernel - Re: [PATCH] Btrfs: work around maybe-uninitialized warning

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:   Fri, 19 May 2017 21:20:53 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     bo.li.liu@...cle.com
Cc:     Chris Mason <clm@...com>, Josef Bacik <jbacik@...com>,
        David Sterba <dsterba@...e.com>,
        Nikolay Borisov <nborisov@...e.com>,
        linux-btrfs <linux-btrfs@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] Btrfs: work around maybe-uninitialized warning

On Fri, May 19, 2017 at 8:10 PM, Liu Bo <bo.li.liu@...cle.com> wrote:
> On Thu, May 18, 2017 at 03:33:29PM +0200, Arnd Bergmann wrote:
>> A rewrite of btrfs_submit_direct_hook appears to have introduced a warning:
>>
>> fs/btrfs/inode.c: In function 'btrfs_submit_direct_hook':
>> fs/btrfs/inode.c:8467:14: error: 'bio' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>
>> Where the 'bio' variable was previously initialized unconditionally, it
>> is now set in the "while (submit_len > 0)" loop that would never execute
>> if submit_len is zero.
>>
>> Assuming this cannot happen in practice, we can avoid the warning
>> by simply replacing the while{} loop with a do{}while() loop so
>> the compiler knows that it will always be entered at least once.
>>
>
> Thanks for the fix.  I think it's a false positve one and I've updated it in v2
> with a 'struct bio *bio = NULL' to make compiler happy, could you please help
> reveiw it?

Right, it is a false positive and adding the =NULL initialization shuts up the
warning. The reason my patch used a different approach is to make the
code more robust, see https://rusty.ozlabs.org/?p=232

Generally speaking initializing a local variable to an illegal value, and later
using the variable without a check for that original value is error-prone.
Even though the code is correct at the moment, someone else might
modify it later. My first (broken) solution avoided this by checking for
the condition that led to the warning, my newer solution is nicer as it
makes it much clearer to the reader what is going on, compared to
the NULL initialization that does not help readability but makes
it slightly harder to understand why you wrote the code specifically that
way.

       Arnd