Fix coverity defects: 147480, 147584 #6745
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
behlendorf
requested changes
Oct 11, 2017
cmd/zed/zed.c
Outdated
| /* Notify parent that daemonization is complete. */ | ||
| zed_log_pipe_close_writes(); | ||
|
|
||
| (void) close(devnull); |
There was a problem hiding this comment.
This isn't quite right. devnull will under normal conditions be larger than STDERR_FILENO so this will be closed on line 202. If that's somehow not that case then one of the dup2() call above must fail. So this actually looks like a false positive to me since Coverity probably doesn't understand the constraints on STD*_FILENO's. Plus adding the close here would result in devnull being closed twice.
There was a problem hiding this comment.
Thanks @behlendorf. I'll try to be more careful. Thanks for you patience.
There was a problem hiding this comment.
It looks like you forgot to drop this hunk from the patch.
There was a problem hiding this comment.
Ouch, I swear I was not being ironic in the comment above.
cmd/zfs/zfs_main.c
Outdated
| usage: | ||
| if (zhp) | ||
| zfs_close(zhp); | ||
| ASSERT(zhp != NULL); |
There was a problem hiding this comment.
You mean ASSERT3P(zhp, ==, NULL); here since zfs_open() must never have been called.
There was a problem hiding this comment.
Next mistake on these macros and I owe you a beer.
tcharding
changed the title
Codecov Report
@@ Coverage Diff @@
## master #6745 +/- ##
==========================================
- Coverage 74.51% 74.11% -0.41%
==========================================
Files 297 295 -2
Lines 94353 93884 -469
==========================================
- Hits 70310 69580 -730
- Misses 24043 24304 +261
Continue to review full report at Codecov.
|
CID 147480: Logically dead code (DEADCODE) Remove non-null check and subsequent function call. Add ASSERT to future proof the code. usage label is only jumped to before `zhp` is initialized. CID 147584: Out-of-bounds access (OVERRUN) Subtract length of current string from buffer length for `size` argument to `snprintf`. Starting address for the write is the start of the buffer + the current string length. We need to subtract this string length else risk a buffer overflow. Signed-off-by: Tobin C. Harding <me@tobin.cc>
behlendorf
approved these changes
Oct 16, 2017
wli5
pushed a commit
to wli5/zfs
that referenced
this pull request
Oct 20, 2017
CID 147480: Logically dead code (DEADCODE) Remove non-null check and subsequent function call. Add ASSERT to future proof the code. usage label is only jumped to before `zhp` is initialized. CID 147584: Out-of-bounds access (OVERRUN) Subtract length of current string from buffer length for `size` argument to `snprintf`. Starting address for the write is the start of the buffer + the current string length. We need to subtract this string length else risk a buffer overflow. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Tobin C. Harding <me@tobin.cc> Closes openzfs#6745
Nasf-Fan
pushed a commit
to Nasf-Fan/zfs
that referenced
this pull request
Nov 4, 2017
CID 147480: Logically dead code (DEADCODE) Remove non-null check and subsequent function call. Add ASSERT to future proof the code. usage label is only jumped to before `zhp` is initialized. CID 147584: Out-of-bounds access (OVERRUN) Subtract length of current string from buffer length for `size` argument to `snprintf`. Starting address for the write is the start of the buffer + the current string length. We need to subtract this string length else risk a buffer overflow. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Tobin C. Harding <me@tobin.cc> Closes openzfs#6745
Nasf-Fan
pushed a commit
to Nasf-Fan/zfs
that referenced
this pull request
Nov 6, 2017
CID 147480: Logically dead code (DEADCODE) Remove non-null check and subsequent function call. Add ASSERT to future proof the code. usage label is only jumped to before `zhp` is initialized. CID 147584: Out-of-bounds access (OVERRUN) Subtract length of current string from buffer length for `size` argument to `snprintf`. Starting address for the write is the start of the buffer + the current string length. We need to subtract this string length else risk a buffer overflow. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Tobin C. Harding <me@tobin.cc> Closes openzfs#6745
Nasf-Fan
pushed a commit
to Nasf-Fan/zfs
that referenced
this pull request
Jan 29, 2018
CID 147480: Logically dead code (DEADCODE) Remove non-null check and subsequent function call. Add ASSERT to future proof the code. usage label is only jumped to before `zhp` is initialized. CID 147584: Out-of-bounds access (OVERRUN) Subtract length of current string from buffer length for `size` argument to `snprintf`. Starting address for the write is the start of the buffer + the current string length. We need to subtract this string length else risk a buffer overflow. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Tobin C. Harding <me@tobin.cc> Closes openzfs#6745
Nasf-Fan
pushed a commit
to Nasf-Fan/zfs
that referenced
this pull request
Feb 13, 2018
CID 147480: Logically dead code (DEADCODE) Remove non-null check and subsequent function call. Add ASSERT to future proof the code. usage label is only jumped to before `zhp` is initialized. CID 147584: Out-of-bounds access (OVERRUN) Subtract length of current string from buffer length for `size` argument to `snprintf`. Starting address for the write is the start of the buffer + the current string length. We need to subtract this string length else risk a buffer overflow. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Tobin C. Harding <me@tobin.cc> Closes openzfs#6745
FransUrbo
pushed a commit
to FransUrbo/zfs
that referenced
this pull request
Apr 28, 2019
CID 147480: Logically dead code (DEADCODE) Remove non-null check and subsequent function call. Add ASSERT to future proof the code. usage label is only jumped to before `zhp` is initialized. CID 147584: Out-of-bounds access (OVERRUN) Subtract length of current string from buffer length for `size` argument to `snprintf`. Starting address for the write is the start of the buffer + the current string length. We need to subtract this string length else risk a buffer overflow. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Tobin C. Harding <me@tobin.cc> Closes openzfs#6745
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CID 147480: Logically dead code (DEADCODE)
Remove non-null check and subsequent function call. Add ASSERT to future
proof the code.
usage label is only jumped to before
zhpis initialized.CID 147584: Out-of-bounds access (OVERRUN)
Subtract length of current string from buffer length for
sizeargumentto
snprintf.Starting address for the write is the start of the buffer + the current
string length. We need to subtract this string length else risk a buffer
overflow.
How Has This Been Tested?
Code is untested.
Types of changes
Checklist:
Signed-off-by.