possible deadcodes in sources

[Petr Lautrbach]

On Mon, Sep 08, 2014 at 01:01:59PM -0600, Eldon Koyle wrote:
> 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.

I'll try to explain why I think that coverity is right.

> >  * 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;
> >  

The original code:

 848         int cmdline = 0, *intptr, value, value2, n, port;
...
 865         intptr = NULL;
...
 886         switch (opcode) {
...
1448         case sAuthorizedPrincipalsFile:
1449                 charptr = &options->authorized_principals_file;
1450                 arg = strdelim(&cp);
1451                 if (!arg || *arg == '\0')
1452                         fatal("%s line %d: missing file name.",
1453                             filename, linenum);
1454                 if (*activep && *charptr == NULL) {
1455                         *charptr = tilde_expand_filename(arg,
getuid());
1456                         /* increase optional counter */
1457                         if (intptr != NULL)
1458                                 *intptr = *intptr + 1;
1459                 }
1460                 break;

- intptr is set to NULL, it's not changed and then is compared if it's not NULL

- intptr is used as a reference to options->num_host_key_files when opcode is sHostKeyFile. While
the it's possible to have more then one hostkey, the authorizedprincipalsfile can be only only. I guess
that lines 1456 - 1458 were jsut cut&pasted from sHostKeyFile block.

> > 
> >  * 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);

 711         int success = 0, tmp1 = -1, tmp2 = -1;
 712 
 713         /* Kludge: ensure there are fds free to receive the pty/tty */
 714         if ((tmp1 = dup(pmonitor->m_recvfd)) == -1 ||
 715             (tmp2 = dup(pmonitor->m_recvfd)) == -1) {
 716                 error("%s: cannot allocate fds for pty", __func__);
 717                 if (tmp1 > 0)
 718                         close(tmp1);
 719                 if (tmp2 > 0)
 720                         close(tmp2);
 721                 return 0;
 722         }

- when first dup(pmonitor->m_recvfd) fails, tmp1 == -1 and tmp2 == -1
- when second dup(pmonitor->m_recvfd) fails, tmp1 is the first unused fd which can be 0 or more and this should
be closed while tmp2 == -1 and there's no action needed


> > 
> >  * 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

1180         int retval = SSH_ERR_INVALID_FORMAT;
...
1317                 retval = 0;
1318 /*XXXX*/
1319                 sshkey_free(k);
1320                 if (retval != 0)
1321                         break;



> >  * clientloop.c:2087:dead_error_line – Execution cannot reach this expression "81" inside statement "packet_start((success ? 81 ...". 
> > 

2075 static void             
2076 client_input_global_request(int type, u_int32_t seq, void *ctxt)
2077 {                       
2078         char *rtype;    
2079         int want_reply;
2080         int success = 0;
2081 
2082         rtype = packet_get_string(NULL);
2083         want_reply = packet_get_char();
2084         debug("client_input_global_request: rtype %s want_reply %d",
2085             rtype, want_reply);
2086         if (want_reply) {       
2087                 packet_start(success ?
2088                     SSH2_MSG_REQUEST_SUCCESS : SSH2_MSG_REQUEST_FAILURE);
2089                 packet_send();
2090                 packet_write_wait(); 
2091         }
2092         free(rtype);    
2093 }


Here I'm not sure if there's missing code or it should simple sent always  SSH2_MSG_REQUEST_FAILURE



-- 
Petr Lautrbach