re: Btrfs: add a extent ref verify tool (static analysis bug report)

re: Btrfs: add a extent ref verify tool (static analysis bug report)

[Colin Ian King]

Hi,

Static analysis on linux-next with Coverity has picked up a potential
issue in file fs/btrfs/ref-verify.c, function process_leaf() in the
following commit:

commit fd708b81d972a0714b02a60eb4792fdbf15868c4
Author: Josef Bacik <josef@toxicpanda.com>
Date:   Fri Sep 29 15:43:50 2017 -0400

    Btrfs: add a extent ref verify tool

The potential issue is when on the unlikely event when all the items
contain unknown key.types and so ret is not assigned a value. Since ret
is not initialized then a garbage value is returned by this function in
this unlikely scenario.

In the previous function process_extent_item any unknown key types are
flagged up as an error and -EINVAL is returned.  I'm unsure if this kind
of error handling should also be applied to function process_leaf with
invalid key types too.

The coverity analysis follows:

495static int process_leaf(struct btrfs_root *root,
496                        struct btrfs_path *path, u64 *bytenr, u64
*num_bytes)
497{
498        struct btrfs_fs_info *fs_info = root->fs_info;
499        struct extent_buffer *leaf = path->nodes[0];
500        struct btrfs_extent_data_ref *dref;
501        struct btrfs_shared_data_ref *sref;

502        u32 count;
   1. var_decl: Declaring variable ret without initializer.

503        int i = 0, tree_block_level = 0, ret;
504        struct btrfs_key key;
505        int nritems = btrfs_header_nritems(leaf);
506
   2. Condition i < nritems, taking true branch.

507        for (i = 0; i < nritems; i++) {
508                btrfs_item_key_to_cpu(leaf, &key, i);
   3. Switch case default.

509                switch (key.type) {
510                case BTRFS_EXTENT_ITEM_KEY:
511                        *num_bytes = key.offset;
512                        /* fall through */
513                case BTRFS_METADATA_ITEM_KEY:
514                        *bytenr = key.objectid;
515                        ret = process_extent_item(fs_info, path, &key, i,
516                                                  &tree_block_level);
517                        break;
518                case BTRFS_TREE_BLOCK_REF_KEY:
519                        ret = add_tree_block(fs_info, key.offset, 0,
520                                             key.objectid,
tree_block_level);
521                        break;
522                case BTRFS_SHARED_BLOCK_REF_KEY:
523                        ret = add_tree_block(fs_info, 0, key.offset,
524                                             key.objectid,
tree_block_level);
525                        break;
526                case BTRFS_EXTENT_DATA_REF_KEY:
527                        dref = btrfs_item_ptr(leaf, i,
528                                              struct
btrfs_extent_data_ref);
529                        ret = add_extent_data_ref(fs_info, leaf,
dref, *bytenr,
530                                                  *num_bytes);
531                        break;
532                case BTRFS_SHARED_DATA_REF_KEY:
533                        sref = btrfs_item_ptr(leaf, i,
534                                              struct
btrfs_shared_data_ref);
535                        count = btrfs_shared_data_ref_count(leaf, sref);
536                        ret = add_shared_data_ref(fs_info,
key.offset, count,
537                                                  *bytenr, *num_bytes);
538                        break;
539                default:
   4. Breaking from switch.

540                        break;
541                }

   CID 19605 (#1 of 1): Uninitialized scalar variable (UNINIT)
   5. uninit_use: Using uninitialized value ret.

542                if (ret)
543                        break;
544        }
545        return ret;
546}

Colin

Re: Btrfs: add a extent ref verify tool (static analysis bug report)

[Josef Bacik]

On Wed, Oct 02, 2019 at 02:50:51PM +0100, Colin Ian King wrote:
> Hi,
> 
> Static analysis on linux-next with Coverity has picked up a potential
> issue in file fs/btrfs/ref-verify.c, function process_leaf() in the
> following commit:
> 
> commit fd708b81d972a0714b02a60eb4792fdbf15868c4
> Author: Josef Bacik <josef@toxicpanda.com>
> Date:   Fri Sep 29 15:43:50 2017 -0400
> 
>     Btrfs: add a extent ref verify tool
> 
> The potential issue is when on the unlikely event when all the items
> contain unknown key.types and so ret is not assigned a value. Since ret
> is not initialized then a garbage value is returned by this function in
> this unlikely scenario.
> 
> In the previous function process_extent_item any unknown key types are
> flagged up as an error and -EINVAL is returned.  I'm unsure if this kind
> of error handling should also be applied to function process_leaf with
> invalid key types too.
> 

Thanks I'll fix this up.  You can run into block group item key types and we
don't care about those, so we just need ret = 0;  Thanks,

Josef