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

Fix get_special_prop() build failure #9020

Merged
merged 1 commit into from
Jul 16, 2019

Conversation

behlendorf
Copy link
Contributor

Motivation and Context

Build failure observed in #8999.

Description

The cast of the size_t returned by strlcpy() to a uint64_t by the
VERIFY3U can result in a build failure when CONFIG_FORTIFY_SOURCE
is set. This is due to the additional hardening. Since the token
will always fit in strval the VERIFY3U has been removed.

How Has This Been Tested?

Locally compiled and tested #8999 (comment)

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)
  • Documentation (a change to man pages or other documentation)

Checklist:

The cast of the size_t returned by strlcpy() to a uint64_t by the
VERIFY3U can result in a build failure when CONFIG_FORTIFY_SOURCE
is set.  This is due to the additional hardening.  Since the token
will always fit in strval the VERIFY3U has been removed.

Signed-off-by: Brian Behlendorf <[email protected]>
Issue openzfs#8999
@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Jul 11, 2019
@behlendorf behlendorf requested a review from don-brady July 11, 2019 17:53
@codecov
Copy link

codecov bot commented Jul 12, 2019

Codecov Report

Merging #9020 into master will decrease coverage by 0.09%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #9020     +/-   ##
=========================================
- Coverage   78.66%   78.56%   -0.1%     
=========================================
  Files         401      401             
  Lines      120146   120146             
=========================================
- Hits        94514    94396    -118     
- Misses      25632    25750    +118
Flag Coverage Δ
#kernel 79.45% <100%> (+0.02%) ⬆️
#user 66.34% <0%> (-0.26%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d230a65...b32ad91. Read the comment docs.

@tonyhutter
Copy link
Contributor

Since the token will always fit in strval the VERIFY3U has been removed.

I looked at get_receive_resume_stats_impl() to see what token's size could be, but it wasn't clear. How do you know it will always fit in strval?

@behlendorf
Copy link
Contributor Author

Technically you're right, we don't have a known hard maximum bound. The token is constructed from a compressed-packed nvlist that is generated, and we happen to know that the nvlist will be relatively small (since we constructed the nvlist). In practice the tokenized ascii version of it is around 200-300 bytes. A lot of redacted snapshots or bookmarks look like they could inflate that a little.

We've also got a lot of headroom here the buffer being allocated is 8192 byte so ~20x what we need. And if we were somehow able to generate a token that large, the worst case would be that a truncated token was returned.

That all said, you may be right there's no good reason not to check for the truncation here and return EOVERFLOW which should bubble up as a fatal lua error when getting properties. And not a truncated token.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jul 16, 2019
@behlendorf behlendorf merged commit 3b03ff2 into openzfs:master Jul 16, 2019
TulsiJain pushed a commit to TulsiJain/zfs that referenced this pull request Jul 20, 2019
The cast of the size_t returned by strlcpy() to a uint64_t by the
VERIFY3U can result in a build failure when CONFIG_FORTIFY_SOURCE
is set.  This is due to the additional hardening.  Since the token
is expected to always fit in strval the VERIFY3U has been removed.
If somehow it doesn't, it will still be safely truncated.

Reviewed-by: Tony Hutter <[email protected]>
Reviewed-by: Don Brady <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Issue openzfs#8999
Closes openzfs#9020
TulsiJain pushed a commit to TulsiJain/zfs that referenced this pull request Jul 20, 2019
The cast of the size_t returned by strlcpy() to a uint64_t by the
VERIFY3U can result in a build failure when CONFIG_FORTIFY_SOURCE
is set.  This is due to the additional hardening.  Since the token
is expected to always fit in strval the VERIFY3U has been removed.
If somehow it doesn't, it will still be safely truncated.

Reviewed-by: Tony Hutter <[email protected]>
Reviewed-by: Don Brady <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Issue openzfs#8999
Closes openzfs#9020
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Aug 13, 2019
The cast of the size_t returned by strlcpy() to a uint64_t by the
VERIFY3U can result in a build failure when CONFIG_FORTIFY_SOURCE
is set.  This is due to the additional hardening.  Since the token
is expected to always fit in strval the VERIFY3U has been removed.
If somehow it doesn't, it will still be safely truncated.

Reviewed-by: Tony Hutter <[email protected]>
Reviewed-by: Don Brady <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Issue openzfs#8999
Closes openzfs#9020
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Aug 21, 2019
The cast of the size_t returned by strlcpy() to a uint64_t by the
VERIFY3U can result in a build failure when CONFIG_FORTIFY_SOURCE
is set.  This is due to the additional hardening.  Since the token
is expected to always fit in strval the VERIFY3U has been removed.
If somehow it doesn't, it will still be safely truncated.

Reviewed-by: Tony Hutter <[email protected]>
Reviewed-by: Don Brady <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Issue openzfs#8999
Closes openzfs#9020
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Aug 22, 2019
The cast of the size_t returned by strlcpy() to a uint64_t by the
VERIFY3U can result in a build failure when CONFIG_FORTIFY_SOURCE
is set.  This is due to the additional hardening.  Since the token
is expected to always fit in strval the VERIFY3U has been removed.
If somehow it doesn't, it will still be safely truncated.

Reviewed-by: Tony Hutter <[email protected]>
Reviewed-by: Don Brady <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Issue openzfs#8999
Closes openzfs#9020
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Aug 23, 2019
The cast of the size_t returned by strlcpy() to a uint64_t by the
VERIFY3U can result in a build failure when CONFIG_FORTIFY_SOURCE
is set.  This is due to the additional hardening.  Since the token
is expected to always fit in strval the VERIFY3U has been removed.
If somehow it doesn't, it will still be safely truncated.

Reviewed-by: Tony Hutter <[email protected]>
Reviewed-by: Don Brady <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Issue openzfs#8999
Closes openzfs#9020
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Sep 17, 2019
The cast of the size_t returned by strlcpy() to a uint64_t by the
VERIFY3U can result in a build failure when CONFIG_FORTIFY_SOURCE
is set.  This is due to the additional hardening.  Since the token
is expected to always fit in strval the VERIFY3U has been removed.
If somehow it doesn't, it will still be safely truncated.

Reviewed-by: Tony Hutter <[email protected]>
Reviewed-by: Don Brady <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Issue openzfs#8999
Closes openzfs#9020
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Sep 18, 2019
The cast of the size_t returned by strlcpy() to a uint64_t by the
VERIFY3U can result in a build failure when CONFIG_FORTIFY_SOURCE
is set.  This is due to the additional hardening.  Since the token
is expected to always fit in strval the VERIFY3U has been removed.
If somehow it doesn't, it will still be safely truncated.

Reviewed-by: Tony Hutter <[email protected]>
Reviewed-by: Don Brady <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Issue openzfs#8999
Closes openzfs#9020
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Sep 23, 2019
The cast of the size_t returned by strlcpy() to a uint64_t by the
VERIFY3U can result in a build failure when CONFIG_FORTIFY_SOURCE
is set.  This is due to the additional hardening.  Since the token
is expected to always fit in strval the VERIFY3U has been removed.
If somehow it doesn't, it will still be safely truncated.

Reviewed-by: Tony Hutter <[email protected]>
Reviewed-by: Don Brady <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Issue openzfs#8999
Closes openzfs#9020
tonyhutter pushed a commit that referenced this pull request Sep 26, 2019
The cast of the size_t returned by strlcpy() to a uint64_t by the
VERIFY3U can result in a build failure when CONFIG_FORTIFY_SOURCE
is set.  This is due to the additional hardening.  Since the token
is expected to always fit in strval the VERIFY3U has been removed.
If somehow it doesn't, it will still be safely truncated.

Reviewed-by: Tony Hutter <[email protected]>
Reviewed-by: Don Brady <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Issue #8999
Closes #9020
@behlendorf behlendorf deleted the fix-zcp_get branch April 19, 2021 19:26
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.

3 participants