btrfs-progs icon indicating copy to clipboard operation
btrfs-progs copied to clipboard
verified publisher icon kdave

Static code analysis of version 5.1 [cleanup][reference]

Open unquietwiki opened this issue 6 years ago • 11 comments

cppcheck 1.86, installed on Ubuntu 18.04.2 LTS; installed uuid-dev, zlib1g-dev, and Kernel 5.1.4 headers.

cppcheck --enable=all --inconclusive --std=c11 -I=/usr/include -I=/usr/src/linux-headers-5.1.4-050104/include/uapi/linux --suppress=missingIncludeSystem -rp . &> ~/cppcheck-186-c11-all.txt

cppcheck-186-c11-all.txt

unquietwiki avatar May 22 '19 22:05 unquietwiki

Looking over it, there are a few instances of unsigned long long, vs long long typecasts, that might explain some of the issues with larger volumes. https://en.wikipedia.org/wiki/C_data_types says there's a difference as of C99. There are also a lot of variable scope issues flagged; which makes it possible for unwanted values to end up in the wrong place.

unquietwiki avatar May 22 '19 22:05 unquietwiki

Thanks, a lot of them look worth fixing. Unifying the printf formats and variable types would be a good start. Variable scope reduction is more a matter of style, can't say we need to clean i up everwhere or not.

I've run cpphceck here, so maybe got other results. Some of the variable shadowing warnings are bogus (check/main.c:2103] -> [check/main.c:1522]: (style) Local variable ret shadows outer variable), but they're in 2 functions. The "%Lx" format is confusing it ([disk-io.c:815]: (warning) %Lx in format string (no. 1) requires 'unsigned long long' but the argument type is 'unsigned long long'.).

kdave avatar Jun 05 '19 18:06 kdave

@kdave My latest copy attempt crapped out after 4 days for NFS/networking glitch, so using this as an opportunity to try fixing this...

unquietwiki avatar Jun 06 '19 00:06 unquietwiki

Making progress.... ran a new analysis after the compiler bonked on my first attempt.

cppcheck --enable=all --force --inconclusive --std=c11 -I /usr/include -I /usr/src/linux-headers-5.1.6-xanmod4/include/uapi/linux --suppress=missingIncludeSystem -rp . &> ~/cppcheck-187-c11-all.txt

References used in first attempt...

  • https://codeforwin.org/2015/05/list-of-all-format-specifiers-in-c-programming.html
  • https://wiki.sei.cmu.edu/confluence/display/c/STR32-C.+Do+not+pass+a+non-null-terminated+character+sequence+to+a+library+function+that+expects+a+string
  • https://en.wikibooks.org/wiki/C_Programming/Pointers_and_arrays

cppcheck-187-c11-all.txt

Side note: took over 2hrs to get the re-analysis. It's still thinking past the 100% mark...

Edit: it returned the part it was waiting on....

[backref.c:328] -> [ctree.c:1158] -> [ctree.c:376]: (error) Null pointer dereference: trans
[qgroup-verify.c:457] -> [qgroup-verify.c:450] -> [qgroup-verify.c:110]: (warning) Null pointer dereference: c
[libbtrfsutil/python/module.c:285]: (style) The function 'PyInit_btrfsutil' is never used.
[tests/sha224-256.c:160]: (style) The function 'SHA224FinalBits' is never used.
[tests/sha224-256.c:135]: (style) The function 'SHA224Input' is never used.
[tests/sha224-256.c:110]: (style) The function 'SHA224Reset' is never used.
[tests/sha224-256.c:185]: (style) The function 'SHA224Result' is never used.
[free-space-cache.c:768]: (style) The function 'btrfs_dump_free_space' is never used.
[btrfs-list.c:1580]: (style) The function 'btrfs_get_subvol' is never used.
[btrfs-list.c:1550]: (style) The function 'btrfs_get_toplevel_subvol' is never used.
[btrfs-list.c:916]: (style) The function 'btrfs_list_get_default_subvolume' is never used.
[inode-item.c:230]: (style) The function 'btrfs_lookup_inode_extref' is never used.
[extent-tree.c:3204]: (style) The function 'btrfs_make_block_groups' is never used.
[fsfeatures.c:127]: (style) The function 'btrfs_process_fs_features' is never used.
[utils.c:2244]: (style) The function 'btrfs_tree_search2_ioctl_supported' is never used.
[kernel-lib/crc32c.c:219]: (style) The function 'crc32c_le' is never used.
[extent_io.c:43]: (style) The function 'extent_io_tree_init_cache_max' is never used.
[backref.c:1652]: (style) The function 'free_ipath' is never used.
[backref.c:1629]: (style) The function 'init_ipath' is never used.
[backref.c:972]: (style) The function 'inode_item_info' is never used.
[mkfs/common.c:776]: (style) The function 'is_vol_small' is never used.
[backref.c:1372]: (style) The function 'iterate_inodes_from_logical' is never used.
[backref.c:1593]: (style) The function 'paths_from_inode' is never used.
[qgroup-verify.c:162]: (style) The function 'print_all_refs' is never used.
[qgroup-verify.c:657]: (style) The function 'print_all_tree_blocks' is never used.
[fsfeatures.c:181]: (style) The function 'print_kernel_version' is never used.
[cmds-balance.c:199]: (style) The function 'print_range' is never used.
[kernel-lib/radix-tree.c:688]: (style) The function 'radix_tree_gang_lookup_tag' is never used.
[kernel-lib/radix-tree.c:351]: (style) The function 'radix_tree_lookup' is never used.
[kernel-lib/radix-tree.c:339]: (style) The function 'radix_tree_lookup_slot' is never used.
[kernel-lib/radix-tree.c:479]: (style) The function 'radix_tree_tag_get' is never used.
[kernel-lib/radix-tree.c:372]: (style) The function 'radix_tree_tag_set' is never used.
[kernel-lib/radix-tree.c:823]: (style) The function 'radix_tree_tagged' is never used.
[kernel-lib/raid56.c:285]: (style) The function 'raid56_recov' is never used.
[utils.c:2540]: (style) The function 'rand_int' is never used.
[utils.c:2555]: (style) The function 'rand_u16' is never used.
[kernel-lib/rbtree.c:542]: (style) The function 'rb_first_postorder' is never used.
[kernel-lib/rbtree.c:524]: (style) The function 'rb_next_postorder' is never used.
[kernel-lib/rbtree.c:496]: (style) The function 'rb_replace_node' is never used.
[free-space-tree.c:1076]: (style) The function 'remove_block_group_free_space' is never used.
[task-utils.c:139]: (style) The function 'task_period_stop' is never used.
[backref.c:1261]: (style) The function 'tree_backref_for_extent' is never used.
[kernel-shared/ulist.c:228]: (style) The function 'ulist_del' is never used.
(information) Cppcheck cannot find all the include files (use --check-config for details)

unquietwiki avatar Jun 06 '19 07:06 unquietwiki

@kdave I finally got it to compile without errors or warnings. I still need to run another cppcheck to verify if I missed anything there. C is not my first language, and the excessive Git activity can be chalked up to syncing with my storage & build system. I tried to notate changes that impacted #59 in separate commits.

unquietwiki avatar Jun 06 '19 22:06 unquietwiki

cppcheck-187-c11-all-v2.txt

cppcheck found 5 warnings & 6 errors after clearing out the compilation alerts. This is definitely in the territory of "stuff that might break"; at least the current set of fixes appear to be working on my restore run (200GB transferred in the past few hours)...

Can't fix

  • [/usr/include/btrfs/kerncompat.h:96]: (warning) %d in format string (no. 2) requires 'int' but the argument type is 'unsigned int'.
  • [/usr/include/btrfs/kerncompat.h:109]: (warning) %d in format string (no. 2) requires 'int' but the argument type is 'unsigned int'.
  • [/usr/include/btrfs/kerncompat.h:313]: (warning) %d in format string (no. 2) requires 'int' but the argument type is 'unsigned int'.

Can try to fix

  • [x] [disk-io.c:130]: (warning) %u in format string (no. 2) requires 'unsigned int' but the argument type is 'signed int'.
  • [x] [qgroup-verify.c:458] -> [qgroup-verify.c:453] -> [qgroup-verify.c:110]: (warning) Null pointer dereference: c
  • [ ] [btrfstune.c:416]: (error, inconclusive) Address of local auto-variable assigned to a function parameter.
  • [ ] [check/main.c:8668]: (error, inconclusive) Address of local auto-variable assigned to a function parameter.
  • [x] [cmds-fi-usage.c:260]: (error) Common realloc mistake: 'sargs' nulled but not freed upon failure
  • [ ] [cmds-receive.c:1159]: (error, inconclusive) Address of local auto-variable assigned to a function parameter.
  • [x] [libbtrfsutil/qgroup.c:61]: (error) Common realloc mistake: 'tmp' nulled but not freed upon failure
  • [x] [backref.c:305] -> [backref.c:329] -> [ctree.c:1154] -> [ctree.c:373]: (error) Null pointer dereference: trans

References

  • https://stackoverflow.com/questions/27589846/dynamic-arrays-using-realloc-without-memory-leaks
  • https://en.wikipedia.org/wiki/Null_pointer (the behavior of *trans in that last item has been out-of-line with other uses).
  • https://en.wikibooks.org/wiki/C_Programming/Memory_management

unquietwiki avatar Jun 07 '19 03:06 unquietwiki

Can't fix

* [/usr/include/btrfs/kerncompat.h:96]: (warning) %d in format string (no. 2) requires 'int' but the argument type is 'unsigned int'.

This can be fixed, kerncompat.h is part of btrfs-progs, but that it includes the system one means you haven't used the define that picks the git headers and not from system. BTRFS_FLAT_INCLUDES

kdave avatar Jun 14 '19 13:06 kdave

@kdave Checking in: restore of over 100TB nearing completion. RAM usage seems to have peaked at ~11GB; not sure if that's from it keeping track of all it has restored (if it's restoring serially, then I can't see it using that much RAM), or an actual memory leak.

unquietwiki avatar Jul 18 '19 01:07 unquietwiki

The memory usage scales with size of metadata, for a 100TB filesystem I'd say 11G is not that bad.

kdave avatar Jul 30 '19 14:07 kdave

@kdave That makes sense. btw, the recovery finished a few days ago, and the memory did de-allocate after the run.

unquietwiki avatar Jul 30 '19 18:07 unquietwiki

@kdave hey, I was looking this issue up for someone asking me about it, and realized it was still open. Should we close this, since you said you had integrated some code fixes a while back? Hope all is well.

unquietwiki avatar Jul 19 '21 22:07 unquietwiki