Coverity fixes #238
Coverity fixes #238
Conversation
|
Some of these have been fixed (eg the addr.c and ssh-keygen.c moduli screening ones). Could you please update and rebase? |
|
@daztucker done. |
|
Whoops, sorry, a typo :( |
|
Fixup pushed |
beldmit
force-pushed
the
coverity_85
branch
3 times, most recently
from
bb98864
to
5f9b431
Compare
|
The tests has passed locally. |
|
The failures seem irrelevant |
openbsd-compat/bindresvport.c
Outdated
| @@ -1,4 +1,4 @@ | |||
| /* This file has be substantially modified from the original OpenBSD source */ | |||
| * This file has be substantially modified from the original OpenBSD source */ | |||
| @@ -1261,6 +1261,7 @@ is_key_revoked(struct ssh_krl *krl, const struct sshkey *key) | |||
| return r; | |||
| erb = RB_FIND(revoked_blob_tree, &krl->revoked_sha1s, &rb); | |||
| free(rb.blob); | |||
| rb.blob = NULL; /* make coverity happy */ | |||
There was a problem hiding this comment.
what is the reason for this change? (our Coverity scan doesn't flag this)
| @@ -436,9 +436,9 @@ channel_register_fds(struct ssh *ssh, Channel *c, int rfd, int wfd, int efd, | |||
| } else if (nonblock) { | |||
| if (rfd != -1) | |||
| set_nonblock(rfd); | |||
| if (wfd != -1) | |||
There was a problem hiding this comment.
what warning is this fixing, and why is rfd treated differently?
There was a problem hiding this comment.
I think it's an artifact of our coverity run, they should be uniform.
| @@ -543,7 +543,7 @@ server_request_tun(struct ssh *ssh) | |||
| debug_f("invalid tun"); | |||
| goto done; | |||
| } | |||
| if (auth_opts->force_tun_device != -1) { | |||
| if (auth_opts->force_tun_device >= 0) { | |||
There was a problem hiding this comment.
Sorry, don't remember. Better skip it.
| ssh_sandbox_child(box); | ||
| free(box); |
There was a problem hiding this comment.
sent upstream, waiting for an ok.
There was a problem hiding this comment.
Damien points out that this will cause problems for other sandboxes where the sandbox has some persistent (non-kernel) state, eg the systrace sandbox since the contents of "box" is needed throughout the life of the preauth privsep supervisor process.
Ideally we'd add a ssh_sandbox_child_finish and call it near mm_send_keystate, but the "box" variable doesn't exist in that scope and getting it there would be messy.
One option would be to free "box" at the end of ssh_sandbox_child in any sandbox implementation that can do that, and do nothing for the ones that can't. Another option would be to just ignore it, because while it is technically a leak, it's in a process that exists only during the authentication process and exits once the authentication succeeds or fails.
|
Resolved, thanks. What's better - to remove the places where the changes are suspicious? |
This is a set of patches fixing the issues found during the coverity scan of the openssh 8.5.