Fix Regressions caught by CodeQL and Coverity #14573

ryao · 2023-03-04T23:52:02Z

Motivation and Context

A few recently merged patches introduced regressions.

4c5fec0 introduced a regression caught by Coverity.

dc5c800 introduced a regression caught by CodeQL in a development branch where I am tuning a more aggressive CodeQL configuration than the one we run on pull requests.

Description

The individual patches have descriptions, but in summary, we switch from ASSERT3U() to ASSERT3S() in one and add a missing loop counter increment in another.

How Has This Been Tested?

The buildbot can test it.

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.

ryao · 2023-03-04T23:53:06Z

@amotin You might want to re-do your performance evaluation of dc5c800 with this fix in place.

@mcmilk Please review the commit touching your SHA2 work.

ryao · 2023-03-05T01:05:12Z

I just changed the title of one of the commits. The commit is otherwise identical to the earlier version.

mcmilk · 2023-03-05T06:32:38Z

module/icp/algs/sha2/sha2_generic.c

-	ASSERT3U(algotype, >=, SHA256_MECH_INFO_TYPE);
-	ASSERT3U(algotype, <=, SHA512_256_MECH_INFO_TYPE);
+	ASSERT3S(algotype, >=, SHA256_MECH_INFO_TYPE);
+	ASSERT3S(algotype, <=, SHA512_256_MECH_INFO_TYPE);



@ryao - Oh yes, you are right.

The recent 4c5fec0 commit caused Coverity to report that ASSERT3U(algotype, >=, SHA256_MECH_INFO_TYPE); is always true. That is because the signed algotype and signed SHA256_MECH_INFO_TYPE values were cast to unsigned types. To fix this, we switch the assertions to use ASSERT3S(), which retains the signedness of the original values for the comparison. Reported-by: Coverity (CID-1535300) Signed-off-by: Richard Yao <[email protected]>

dc5c800 was recently merged to prefetch up to 128 deadlists. Unfortunately, a loop was missing an increment, such that it will prefetch all deadlists. The performance properties of that patch probably should be re-evaluated. This was caught by CodeQL's cpp/constant-comparison check in an experimental branch where I am testing the security-and-extended queries. It complained about the `i < 128` part of the loop condition always evaluating to the same thing. The standard CodeQL configuration we use missed this because it does not include that check. Signed-off-by: Richard Yao <[email protected]>

ryao · 2023-03-06T02:03:46Z

I changed a commit title and repushed.

ryao · 2023-03-06T19:04:58Z

@behlendorf The zloop failure is a pre-existing issue. I opened #14583 with a fix for it. As for the Fedora issue, I suspect it is a pre-existing issue too, but the telemetry is not good enough for me to see why.

ryao · 2023-03-06T22:33:05Z

The Fedora buildbot failure is #14584.

dc5c800 was recently merged to prefetch up to 128 deadlists. Unfortunately, a loop was missing an increment, such that it will prefetch all deadlists. The performance properties of that patch probably should be re-evaluated. This was caught by CodeQL's cpp/constant-comparison check in an experimental branch where I am testing the security-and-extended queries. It complained about the `i < 128` part of the loop condition always evaluating to the same thing. The standard CodeQL configuration we use missed this because it does not include that check. Reviewed-by: Tino Reichardt <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes #14573

dc5c800 was recently merged to prefetch up to 128 deadlists. Unfortunately, a loop was missing an increment, such that it will prefetch all deadlists. The performance properties of that patch probably should be re-evaluated. This was caught by CodeQL's cpp/constant-comparison check in an experimental branch where I am testing the security-and-extended queries. It complained about the `i < 128` part of the loop condition always evaluating to the same thing. The standard CodeQL configuration we use missed this because it does not include that check. Reviewed-by: Tino Reichardt <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14573

dc5c800 was recently merged to prefetch up to 128 deadlists. Unfortunately, a loop was missing an increment, such that it will prefetch all deadlists. The performance properties of that patch probably should be re-evaluated. This was caught by CodeQL's cpp/constant-comparison check in an experimental branch where I am testing the security-and-extended queries. It complained about the `i < 128` part of the loop condition always evaluating to the same thing. The standard CodeQL configuration we use missed this because it does not include that check. Reviewed-by: Tino Reichardt <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes #14573

The recent 4c5fec0 commit caused Coverity to report that ASSERT3U(algotype, >=, SHA256_MECH_INFO_TYPE); is always true. That is because the signed algotype and signed SHA256_MECH_INFO_TYPE values were cast to unsigned types. To fix this, we switch the assertions to use ASSERT3S(), which retains the signedness of the original values for the comparison. Reviewed-by: Tino Reichardt <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Signed-off-by: Richard Yao <[email protected]> Reported-by: Coverity (CID-1535300) Closes openzfs#14573

dc5c800 was recently merged to prefetch up to 128 deadlists. Unfortunately, a loop was missing an increment, such that it will prefetch all deadlists. The performance properties of that patch probably should be re-evaluated. This was caught by CodeQL's cpp/constant-comparison check in an experimental branch where I am testing the security-and-extended queries. It complained about the `i < 128` part of the loop condition always evaluating to the same thing. The standard CodeQL configuration we use missed this because it does not include that check. Reviewed-by: Tino Reichardt <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14573

The recent 4c5fec0 commit caused Coverity to report that ASSERT3U(algotype, >=, SHA256_MECH_INFO_TYPE); is always true. That is because the signed algotype and signed SHA256_MECH_INFO_TYPE values were cast to unsigned types. To fix this, we switch the assertions to use ASSERT3S(), which retains the signedness of the original values for the comparison. Reviewed-by: Tino Reichardt <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Signed-off-by: Richard Yao <[email protected]> Reported-by: Coverity (CID-1535300) Closes openzfs#14573

dc5c800 was recently merged to prefetch up to 128 deadlists. Unfortunately, a loop was missing an increment, such that it will prefetch all deadlists. The performance properties of that patch probably should be re-evaluated. This was caught by CodeQL's cpp/constant-comparison check in an experimental branch where I am testing the security-and-extended queries. It complained about the `i < 128` part of the loop condition always evaluating to the same thing. The standard CodeQL configuration we use missed this because it does not include that check. Reviewed-by: Tino Reichardt <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14573

ryao requested a review from amotin March 4, 2023 23:52

amotin approved these changes Mar 5, 2023

View reviewed changes

ryao force-pushed the regressions branch from ec46207 to 10adb55 Compare March 5, 2023 01:04

mcmilk approved these changes Mar 5, 2023

View reviewed changes

ryao added 2 commits March 5, 2023 21:02

ryao force-pushed the regressions branch from 10adb55 to da34b67 Compare March 6, 2023 02:03

behlendorf closed this in 8846139 Mar 6, 2023

behlendorf mentioned this pull request Mar 7, 2023

[2.1.10] Add missing increment to dsl_deadlist_move_bpobj() #14587

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix Regressions caught by CodeQL and Coverity #14573

Fix Regressions caught by CodeQL and Coverity #14573

Uh oh!

ryao commented Mar 4, 2023

Uh oh!

ryao commented Mar 4, 2023 •

edited

Loading

Uh oh!

ryao commented Mar 5, 2023

Uh oh!

mcmilk Mar 5, 2023

Uh oh!

ryao commented Mar 6, 2023

Uh oh!

ryao commented Mar 6, 2023

Uh oh!

ryao commented Mar 6, 2023

Uh oh!

Uh oh!

Fix Regressions caught by CodeQL and Coverity #14573

Fix Regressions caught by CodeQL and Coverity #14573

Uh oh!

Conversation

ryao commented Mar 4, 2023

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

ryao commented Mar 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryao commented Mar 5, 2023

Uh oh!

mcmilk Mar 5, 2023

Choose a reason for hiding this comment

Uh oh!

ryao commented Mar 6, 2023

Uh oh!

ryao commented Mar 6, 2023

Uh oh!

ryao commented Mar 6, 2023

Uh oh!

Uh oh!

ryao commented Mar 4, 2023 •

edited

Loading