Bug 205003 - possible null pointer dereferences
Summary: possible null pointer dereferences
Alias: None
Product: File System
Classification: Unclassified
Component: btrfs (show other bugs)
Hardware: All Linux
: P1 normal
Assignee: BTRFS virtual assignee
Depends on:
Reported: 2019-09-26 06:39 UTC by p
Modified: 2019-12-05 14:23 UTC (History)
2 users (show)

See Also:
Kernel Version: 5.4, 4.19
Regression: No
Bisected commit-id:


Description p 2019-09-26 06:39:11 UTC
Found by cppcheck.

In btrfsic_process_superblock() @ check-integrity.c:
null check at line 640 after dereference at line 632

In btrfs_destroy_dev_replace_tgtdev() @ volumes.c:
null check at line 2310 after dereference at line 2308

__readahead_hook() at reada.c passes potentially null argument eb (according to
the function doc) to these functions, which dereference eb:
btrfs_header_generation(), btrfs_header_nritems(),

btrfs_header_* are defined in ctree.h through BTRFS_SETGET_HEADER_FUNCS.

reada_start_machine_dev() actually calls __readahead_hook() with eb = NULL.
Comment 1 Johannes Thumshirn 2019-12-05 13:12:45 UTC
Case Number 1 cannot happen as the only caller of btrfsic_process_superblock() is btrfsic_mount() and it allocates 'state' before calling btrfsic_process_superblock() and does proper error handling. 
Therefore we can a) remove the BUG_ON(NULL == state) completely and we can remove the local fs_info variable as well. I have patches doing both and will submit them soon.

For case number 2, it is also impossible to pass a NULL tgtdev into btrfs_destroy_dev_replace_tgtdev(), so the NULL pointer check is superfluous. I do also have a patch in the queue to remove this check.

For case number 3, this is also a false positive as we're never calling any of the btrfs_header_XXX() functions with a NULL extent_buffer. In all cases where eb could be NULL we're also setting the 'err' variable and __readahead_hook() has the following hunk in the beginning of the function:

>        /*
>         * this is the error case, the extent buffer has not been
>         * read correctly. We won't access anything from it and
>         * just cleanup our data structures. Effectively this will
>         * cut the branch below this node from read ahead.
>         */
>        if (err)
>                goto cleanup;

The patches silencing the static checkers for cases 1 and 2 can be found in:
Comment 2 David Sterba 2019-12-05 14:23:51 UTC
Patches added to misc-next, thanks.

Note You need to log in before you can comment on or make changes to this bug.