I think coverity is probably wrong on some of these. Code coverage testing is a hard problem, and it is not uncommon for the tools to get it wrong. There wasn't enough context in servconf.c to see if there is actually a coverage issue, but the patch completely changes the behavior. In monitor_wrap.c, it looks like your test might be assuming that two calls to dup() will return the same value, which is definitely not the case. For the one in sshkey.c, you need to check to see if retval is a global and also check to see if sshkey_free() modifies it; just because it isn't in the argument list doesn't mean there isn't a side-effect. Here is an example of why not to blindly trust code checking tools: https://www.schneier.com/blog/archives/2008/05/random_number_b.html Not saying it isn't worth looking at, just saying you need to keep in mind the possibility that they are completely wrong. -- Eldon Koyle -- "I don't care who does the electing as long as I get to do the nominating." -- Boss Tweed On Sep 08 20:31+0200, Petr Lautrbach wrote: > Hello, > > we've run a coverity scan on the openssh sources and it found several > issues. Although the scan was run on patched rhel sources, some results are applicable to vanilla sources > too. > > * servconf.c:1458:dead_error_line – Execution cannot reach this statement "*intptr = *intptr + 1;" > > --- a/servconf.c > +++ b/servconf.c > @@ -1451,12 +1451,8 @@ process_server_config_line(ServerOptions *options, char *line, > if (!arg || *arg == '\0') > fatal("%s line %d: missing file name.", > filename, linenum); > - if (*activep && *charptr == NULL) { > + if (*activep && *charptr == NULL) > *charptr = tilde_expand_filename(arg, getuid()); > - /* increase optional counter */ > - if (intptr != NULL) > - *intptr = *intptr + 1; > - } > break; > > case sClientAliveInterval: > > > > * monitor_wrap.c:720:dead_error_line – Execution cannot reach this statement "close(tmp2);".o > > --- a/monitor_wrap.c > +++ b/monitor_wrap.c > @@ -714,10 +714,8 @@ mm_pty_allocate(int *ptyfd, int *ttyfd, char *namebuf, size_t namebuflen) > if ((tmp1 = dup(pmonitor->m_recvfd)) == -1 || > (tmp2 = dup(pmonitor->m_recvfd)) == -1) { > error("%s: cannot allocate fds for pty", __func__); > - if (tmp1 > 0) > + if (tmp1 > -1) > close(tmp1); > - if (tmp2 > 0) > - close(tmp2); > return 0; > } > close(tmp1); > > > * sshkey.c:1321:dead_error_line – Execution cannot reach this statement "break;". > > code: > retval = 0; > /*XXXX*/ > sshkey_free(k); > if (retval != 0) > break; > > XXXX here probably means fix in future, but the last two lines seem to be functionless > > > > * clientloop.c:2087:dead_error_line – Execution cannot reach this expression "81" inside statement "packet_start((success ? 81 ...". > > > > > I hope that it makes sense. > > -- > Petr Lautrbach > _______________________________________________ > openssh-unix-dev mailing list > openssh-unix-dev@xxxxxxxxxxx > https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev _______________________________________________ openssh-unix-dev mailing list openssh-unix-dev@xxxxxxxxxxx https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
