Minor cleanup to suppress static analyzer complaints #14042

ryao · 2022-10-17T06:35:03Z

Motivation and Context

Various little clean-up opportunities were caught by static analyzers. They are being done to suppress static analyzer complaints.

Description

Each commit message describes the change. I left them as separate commits because I did not see a clean way of documenting each change in a squashed commit message.

How Has This Been Tested?

Build tests have been done.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)
Breaking change (fix or feature that would cause existing functionality to change)
Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the OpenZFS code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
I have run the ZFS Test Suite with this change applied.
All commit messages are properly formatted and contain Signed-off-by.

Clang's static analyzer complained that we could use after free here if the inner loop ever iterated. That is a false positive, but upon inspection, the userland abd_alloc_chunks() function never will put multiple consecutive pages into a `struct scatterlist`, so there is no need to loop. We delete the inner loop. Signed-off-by: Richard Yao <[email protected]>

This confused Clang's static analyzer, making it think there was a possible NULL pointer dereference. There is no NULL pointer dereference. Signed-off-by: Richard Yao <[email protected]>

This is a circularly linked list. mg->mg_next can never be NULL. This caused 3 defect reports in Coverity. Signed-off-by: Richard Yao <[email protected]>

It is never NULL because we return early if dsl_pool_hold() fails. This caused Coverity to complain. Signed-off-by: Richard Yao <[email protected]>

module/zfs/dmu_send.c

ryao · 2022-10-17T22:37:42Z

@behlendorf When looking at coverity scans, I noticed one more unnecessary NULL pointer check, so I added it to the PR.

range is always deferenced before it reaches this check, such that the kmem_zalloc() call is never executed. There is also no need to set `range->eos_marker = B_TRUE` because it is already set. Coverity incorrectly complained about a potential NULL pointer dereference because of this. Signed-off-by: Richard Yao <[email protected]>

The pointer is to a structure member, so it is never NULL. Coverity complained about this. Signed-off-by: Richard Yao <[email protected]>

This confused Clang's static analyzer, making it think there was a possible NULL pointer dereference. There is no NULL pointer dereference. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes #14042

This is a circularly linked list. mg->mg_next can never be NULL. This caused 3 defect reports in Coverity. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes #14042

It is never NULL because we return early if dsl_pool_hold() fails. This caused Coverity to complain. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes #14042

range is always deferenced before it reaches this check, such that the kmem_zalloc() call is never executed. There is also no need to set `range->eos_marker = B_TRUE` because it is already set. Coverity incorrectly complained about a potential NULL pointer dereference because of this. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes #14042

The pointer is to a structure member, so it is never NULL. Coverity complained about this. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes #14042

Clang's static analyzer complained that we could use after free here if the inner loop ever iterated. That is a false positive, but upon inspection, the userland abd_alloc_chunks() function never will put multiple consecutive pages into a `struct scatterlist`, so there is no need to loop. We delete the inner loop. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14042

This confused Clang's static analyzer, making it think there was a possible NULL pointer dereference. There is no NULL pointer dereference. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14042

This is a circularly linked list. mg->mg_next can never be NULL. This caused 3 defect reports in Coverity. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14042

It is never NULL because we return early if dsl_pool_hold() fails. This caused Coverity to complain. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14042

range is always deferenced before it reaches this check, such that the kmem_zalloc() call is never executed. There is also no need to set `range->eos_marker = B_TRUE` because it is already set. Coverity incorrectly complained about a potential NULL pointer dereference because of this. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14042

The pointer is to a structure member, so it is never NULL. Coverity complained about this. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14042