Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add defensive assertions #13972

Merged
merged 1 commit into from
Oct 12, 2022
Merged

Conversation

ryao
Copy link
Contributor

@ryao ryao commented Sep 29, 2022

Motivation and Context

Coverity complains about possible bugs involving referencing NULL return
values and division by zero. The division by zero bugs require that a
block pointer be corrupt, either from in-memory corruption, or on-disk
corruption. The NULL return value complaints are only bugs if
assumptions that we make about the state of data structures are wrong.
Some seem impossible to be wrong and thus are false positives, while
others are hard to analyze.

Description

Rather than dismiss these as false positives by assuming we know better, we add defensive assertions to let us know when our assumptions are wrong.

How Has This Been Tested?

A local build test has 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:

@ryao ryao force-pushed the defensive_assertions branch from 2828dc1 to 8a5030e Compare September 29, 2022 17:29
@ryao ryao marked this pull request as draft September 29, 2022 18:51
@ryao ryao force-pushed the defensive_assertions branch from 8a5030e to 49e8f5f Compare October 9, 2022 18:06
Coverity complains about possible bugs involving referencing NULL return
values and division by zero. The division by zero bugs require that a
block pointer be corrupt, either from in-memory corruption, or on-disk
corruption. The NULL return value complaints are only bugs if
assumptions that we make about the state of data structures are wrong.
Some seem impossible to be wrong and thus are false positives, while
others are hard to analyze.

Rather than dismiss these as false positives by assuming we know better,
we add defensive assertions to let us know when our assumptions are
wrong.

Signed-off-by: Richard Yao <[email protected]>
@ryao ryao force-pushed the defensive_assertions branch from 49e8f5f to 095a89b Compare October 9, 2022 23:07
@ryao ryao marked this pull request as ready for review October 9, 2022 23:11
@ryao
Copy link
Contributor Author

ryao commented Oct 9, 2022

It turned out that adding checks to zfs_blkptr_verify() to verify that BP_GET_LSIZE(bp) == 0 and BP_GET_PSIZE(bp) == 0 are never true broke things. I have removed that from the pull request, but if the 0 values for those ever make it to lr->lr_offset / BP_GET_LSIZE(bp), we will have a division by zero. I plan to review that some more, but presumably, the issue is that we are calling zfs_blkptr_verify() in a place where those have yet to be set. Anyway, this pull request is ready to merge as is.

@ryao ryao changed the title Add defensive assertions and block pointer checks Add defensive assertions Oct 9, 2022
@behlendorf behlendorf merged commit a6ccb36 into openzfs:master Oct 12, 2022
@behlendorf behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label Oct 12, 2022
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Oct 12, 2022
Coverity complains about possible bugs involving referencing NULL return
values and division by zero. The division by zero bugs require that a
block pointer be corrupt, either from in-memory corruption, or on-disk
corruption. The NULL return value complaints are only bugs if
assumptions that we make about the state of data structures are wrong.
Some seem impossible to be wrong and thus are false positives, while
others are hard to analyze.

Rather than dismiss these as false positives by assuming we know better,
we add defensive assertions to let us know when our assumptions are
wrong.

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Richard Yao <[email protected]>
Closes openzfs#13972
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants