| Message ID | 20201031010752.23974-5-wqu@suse.com |
|---|---|
| State | Rejected |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | fs: btrfs: coverity fixes | expand |
On Sat, 31 Oct 2020 09:07:52 +0800 Qu Wenruo <wqu@suse.com> wrote: > In show_dir() if we hit file type FT_CHRDEV or FT_BLKDEV, we expect an > BTRFS_INODE_ITEM_KEY, and for that case, we should have @ii filled with > data read from disk. > > We even have ASSERT() for this purpose, but unfortunately coverity can't > understand the ASSERT() nor if we get key type BTRFS_INODE_ITEM_KEY, > previous if() branch must go to the read_extent_buffer() branch to fill > the @ii. > > So to make coverity happy, just initialize the variable @ii to all zero. WTF. If ASSERT excludes this from happening, coverity should understand this. We live in 2020. I don't much like the idea to do things just to get coverity happy if it can't understand and infer from things like ASSERT.
On 2020/11/2 上午7:06, Marek Behun wrote: > On Sat, 31 Oct 2020 09:07:52 +0800 > Qu Wenruo <wqu@suse.com> wrote: > >> In show_dir() if we hit file type FT_CHRDEV or FT_BLKDEV, we expect an >> BTRFS_INODE_ITEM_KEY, and for that case, we should have @ii filled with >> data read from disk. >> >> We even have ASSERT() for this purpose, but unfortunately coverity can't >> understand the ASSERT() nor if we get key type BTRFS_INODE_ITEM_KEY, >> previous if() branch must go to the read_extent_buffer() branch to fill >> the @ii. >> >> So to make coverity happy, just initialize the variable @ii to all zero. > > WTF. If ASSERT excludes this from happening, coverity should understand > this. We live in 2020. I don't much like the idea to do things just to > get coverity happy if it can't understand and infer from things like > ASSERT. > It's not ASSERT(), it's the previous if () branch. The ASSERT() is just to ensure we didn't miss anything. The previous if () looks like this: if (key.type == BTRFS_ROOT_ITEM_KEY) { /* @ii is not touched */ /* Further more, in this branch type should only be FT_DIR */ } else { /* This includes the key.type == INODE_ITEM_KEY case */ read_extent_buffer( &ii ); /* Here we fill &ii */ } Then in the offending code we have: if (type == BTRFS_FT_CHRDEV || type == BTRFS_FT_BLKDEV) { ASSERT(key.type == BTRFS_INODE_ITEM_KEY); /* * If the type is above type, we must have key type INODE_ITEM * Thus the ii must be filled. */ printf("%4llu,%5llu ", btrfs_stack_inode_rdev(&ii) >> 20, btrfs_stack_inode_rdev(&ii) & 0xfffff); } else { if (key.type == BTRFS_INODE_ITEM_KEY) printf("%10llu ", btrfs_stack_inode_size(&ii)); else printf("%10llu ", 0ULL); } Thus I really tend to believe it's just a bug in coverity. All locations accessing @ii all have its key.type checked to ensure it get filled in the first place. Thanks, Qu
On Mon, 2 Nov 2020 08:27:16 +0800 Qu Wenruo <wqu@suse.com> wrote: > Thus I really tend to believe it's just a bug in coverity. > All locations accessing @ii all have its key.type checked to ensure it get filled in the first place. If this is a bug in coverity, we should fix coverity, not add extra code to U-Boot, even if it is just one instruction. That simply stinks the same way like when systemd crashed when "debug" parameter was present in /proc/cmdline, and they sent a patch to kernel which removed the "debug" parameter, instead of fixing systemd. IMO. Marek
On 2020/11/2 下午3:24, Marek Behun wrote: > On Mon, 2 Nov 2020 08:27:16 +0800 > Qu Wenruo <wqu@suse.com> wrote: > >> Thus I really tend to believe it's just a bug in coverity. >> All locations accessing @ii all have its key.type checked to ensure it get filled in the first place. > > If this is a bug in coverity, we should fix coverity, not add extra > code to U-Boot, even if it is just one instruction. That simply stinks > the same way like when systemd crashed when "debug" parameter was > present in /proc/cmdline, and they sent a patch to kernel which removed > the "debug" parameter, instead of fixing systemd. IMO. Yep, you're right. Please discard this patch. Thanks, Qu > > Marek >
On Mon, Nov 02, 2020 at 08:24:09AM +0100, Marek Behun wrote: > On Mon, 2 Nov 2020 08:27:16 +0800 > Qu Wenruo <wqu@suse.com> wrote: > > > Thus I really tend to believe it's just a bug in coverity. > > All locations accessing @ii all have its key.type checked to ensure it get filled in the first place. > > If this is a bug in coverity, we should fix coverity, not add extra > code to U-Boot, even if it is just one instruction. That simply stinks > the same way like when systemd crashed when "debug" parameter was > present in /proc/cmdline, and they sent a patch to kernel which removed > the "debug" parameter, instead of fixing systemd. IMO. To be clear, I am quite happy to mark as intentional any issues that are flagged by Coverity but, well, intentional and not a problem. We may also benefit from adding a "modeling" file to help Coverity know when some cases of issues are handled by us already.
diff --git a/fs/btrfs/btrfs.c b/fs/btrfs/btrfs.c index 346b2c4341c2..1abd2c291177 100644 --- a/fs/btrfs/btrfs.c +++ b/fs/btrfs/btrfs.c @@ -19,7 +19,7 @@ static int show_dir(struct btrfs_root *root, struct extent_buffer *eb, struct btrfs_dir_item *di) { struct btrfs_fs_info *fs_info = root->fs_info; - struct btrfs_inode_item ii; + struct btrfs_inode_item ii = { 0 }; struct btrfs_key key; static const char* dir_item_str[] = { [BTRFS_FT_REG_FILE] = "FILE",
In show_dir() if we hit file type FT_CHRDEV or FT_BLKDEV, we expect an BTRFS_INODE_ITEM_KEY, and for that case, we should have @ii filled with data read from disk. We even have ASSERT() for this purpose, but unfortunately coverity can't understand the ASSERT() nor if we get key type BTRFS_INODE_ITEM_KEY, previous if() branch must go to the read_extent_buffer() branch to fill the @ii. So to make coverity happy, just initialize the variable @ii to all zero. Reported-by: Coverity CID 312950 Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/btrfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)