possible deadcodes in sources

[Eldon Koyle]

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 at mindrot.org
> https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev