Miscellaneous fixes #14264

ryao · 2022-12-04T20:07:04Z

Motivation and Context

These potential fix runtime issues found when doing static analysis.

Description

The individual commit messages describe the changes.

How Has This Been Tested?

They have been build tested. The buildbot can verify that there are no regressions.

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.

The ZPOOL_SCRIPTS_PATH environment variable can be passed here. This allows for arbitrarily long strings to be passed to sprintf(), which can overflow the buffer. I missed this in my earlier audit of the codebase. CodeQL's cpp/unbounded-write check caught this. Signed-off-by: Richard Yao <[email protected]>

8889144 introduced this regression. I used cscope to verify that there are no other instances of this in the codebase. This is the one of the few bugs that are extremely easy to identify using cscope. Signed-off-by: Richard Yao <[email protected]>

This was written by Jeff Bonick and was committed to OpenSolaris on November 1, 2009. It appears that Jeff meant to continue the outer loop iteration when `ddp->ddp_phys_birth == 0`, but put his check inside the inner loop. This causes a pointer to uninitialized memory to be passed to ddt_lookup() inside a VERIFY() statement whenever that condition is true. Reported-by: Coverity (CID 1524462) Signed-off-by: Richard Yao <[email protected]>

ryao · 2022-12-05T15:04:28Z

The repush fixes checkstyle complaints.

cmd/zdb/zdb.c

CodeQL pointed out that for extreme floating point values, `sprintf()` will overwrite a 32 character buffer. It cited 1e304 as an example, which causes `sprintf()` to print 308 characters. In practice, the numbers should never exceed 100, so this should not happen. To silence the warning and also handle unexpected situations, we change the code to use `snprintf()`. This was missed during my audit of our use of `sprintf()`, since I did not think to consider extreme floating point representations. It also really should not happen, so this change is purely defensive programming. This was found by CodeQL's cpp/overrunning-write-with-float check. Signed-off-by: Richard Yao <[email protected]>

If the bp is NULL, we have a hole. However, when we build with assertions, we will dereference bp when `blkid == DMU_SPILL_BLKID`. When this happens on a hole, we will have a NULL pointer dereference. Reported-by: Coverity (CID-1524670) Signed-off-by: Richard Yao <[email protected]>

`zfs_send_cb_impl()` calls `dump_filesystems()`, which calls `dump_filesystem()`, which will return `-1` as an error when `zfs_open()` returns `NULL`. This will be passed to `zfs_standard_error()`, which passes it to `zfs_standard_error_fmt()`, which passes it to `strerror()`. To fix this, we modify zfs_open() to set `errno` whenever it returns NULL. Most of the cases already have `errno` set (since they pass it to `zfs_standard_error_fmt()`, which makes this easy. Then we modify `dump_filesystem()` to pass `errno` instead of `-1`. Reported-by: Coverity (CID-1524598) Signed-off-by: Richard Yao <[email protected]>

ryao · 2022-12-05T20:18:56Z

CodeQL has a query called cpp/assign-where-compare-meant that should catch (errno = EINVAL) on paper. However, it neither is turned on by default nor caught it when I tested turning it on yesterday. An issue will be filed with the CodeQL developers at some point in the future when I have enough information to make a good description of the false negative.

8889144 introduced this regression. I used cscope to verify that there are no other instances of this in the codebase. This is the one of the few bugs that are extremely easy to identify using cscope. Reviewed-by: Damian Szuberski <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes #14264

This was written by Jeff Bonick and was committed to OpenSolaris on November 1, 2009. It appears that Jeff meant to continue the outer loop iteration when `ddp->ddp_phys_birth == 0`, but put his check inside the inner loop. This causes a pointer to uninitialized memory to be passed to ddt_lookup() inside a VERIFY() statement whenever that condition is true. Reported-by: Coverity (CID 1524462) Reviewed-by: Damian Szuberski <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes #14264

CodeQL pointed out that for extreme floating point values, `sprintf()` will overwrite a 32 character buffer. It cited 1e304 as an example, which causes `sprintf()` to print 308 characters. In practice, the numbers should never exceed 100, so this should not happen. To silence the warning and also handle unexpected situations, we change the code to use `snprintf()`. This was missed during my audit of our use of `sprintf()`, since I did not think to consider extreme floating point representations. It also really should not happen, so this change is purely defensive programming. This was found by CodeQL's cpp/overrunning-write-with-float check. Reviewed-by: Damian Szuberski <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes #14264

If the bp is NULL, we have a hole. However, when we build with assertions, we will dereference bp when `blkid == DMU_SPILL_BLKID`. When this happens on a hole, we will have a NULL pointer dereference. Reported-by: Coverity (CID-1524670) Reviewed-by: Damian Szuberski <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes #14264

`zfs_send_cb_impl()` calls `dump_filesystems()`, which calls `dump_filesystem()`, which will return `-1` as an error when `zfs_open()` returns `NULL`. This will be passed to `zfs_standard_error()`, which passes it to `zfs_standard_error_fmt()`, which passes it to `strerror()`. To fix this, we modify zfs_open() to set `errno` whenever it returns NULL. Most of the cases already have `errno` set (since they pass it to `zfs_standard_error_fmt()`, which makes this easy. Then we modify `dump_filesystem()` to pass `errno` instead of `-1`. Reported-by: Coverity (CID-1524598) Reviewed-by: Damian Szuberski <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes #14264

The ZPOOL_SCRIPTS_PATH environment variable can be passed here. This allows for arbitrarily long strings to be passed to sprintf(), which can overflow the buffer. I missed this in my earlier audit of the codebase. CodeQL's cpp/unbounded-write check caught this. Reviewed-by: Damian Szuberski <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14264

8889144 introduced this regression. I used cscope to verify that there are no other instances of this in the codebase. This is the one of the few bugs that are extremely easy to identify using cscope. Reviewed-by: Damian Szuberski <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14264

This was written by Jeff Bonick and was committed to OpenSolaris on November 1, 2009. It appears that Jeff meant to continue the outer loop iteration when `ddp->ddp_phys_birth == 0`, but put his check inside the inner loop. This causes a pointer to uninitialized memory to be passed to ddt_lookup() inside a VERIFY() statement whenever that condition is true. Reported-by: Coverity (CID 1524462) Reviewed-by: Damian Szuberski <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14264

CodeQL pointed out that for extreme floating point values, `sprintf()` will overwrite a 32 character buffer. It cited 1e304 as an example, which causes `sprintf()` to print 308 characters. In practice, the numbers should never exceed 100, so this should not happen. To silence the warning and also handle unexpected situations, we change the code to use `snprintf()`. This was missed during my audit of our use of `sprintf()`, since I did not think to consider extreme floating point representations. It also really should not happen, so this change is purely defensive programming. This was found by CodeQL's cpp/overrunning-write-with-float check. Reviewed-by: Damian Szuberski <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14264

If the bp is NULL, we have a hole. However, when we build with assertions, we will dereference bp when `blkid == DMU_SPILL_BLKID`. When this happens on a hole, we will have a NULL pointer dereference. Reported-by: Coverity (CID-1524670) Reviewed-by: Damian Szuberski <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14264

`zfs_send_cb_impl()` calls `dump_filesystems()`, which calls `dump_filesystem()`, which will return `-1` as an error when `zfs_open()` returns `NULL`. This will be passed to `zfs_standard_error()`, which passes it to `zfs_standard_error_fmt()`, which passes it to `strerror()`. To fix this, we modify zfs_open() to set `errno` whenever it returns NULL. Most of the cases already have `errno` set (since they pass it to `zfs_standard_error_fmt()`, which makes this easy. Then we modify `dump_filesystem()` to pass `errno` instead of `-1`. Reported-by: Coverity (CID-1524598) Reviewed-by: Damian Szuberski <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14264