Re: Analysis of clang results for nmap main directory.

[James Rogers <jamesmrogers () gmail com>]

On Thu, Jun 14, 2012 at 1:42 PM, David Fifield <david () bamsoftware com> wrote:
> On Thu, Jun 14, 2012 at 01:06:01PM -0400, James Rogers wrote:
> > Reran clang against latest version of nmap on Tuesday, many changes
> > had been made since I first ran this tool and I wanted the results to
> > be as meaningful as possible.
> >
> > The rest of the bugs are just an assignment or increment of a local
> > variable and the variable never being used again after the last
> > change.  Not actual bugs, but good coding practices discourge setting
> > variable you never use again.
>
> I have comments on a few of these.
>
> > Dead store    Dead assignment                                         output.cc               2160
> > 2160  delim = "; ";
> >       Value stored to 'delim' is never read
> >       Bug, line can be removed with affecting program.
>
> I don't think this line should be removed. Look at the preceding code:
> it's part of a symmetric structure of similar code blocks. Imagine what
> happens if another block is added at the end; the lack of an assignment
> to delim would be a bug. This is sort of like defensively putting a
> "break" after your last "switch" case; it is useless but it protects you
> when you add another case. Maybe this whole section can be rewritten to
> be clearer, but as it stands I think the assignment should remain.

Maybe the symmetry could be

delim="; ";
block of code

in the preceding sections.  If someone copies and pastes a block to
the end, then it would still work.

> > Dead store    Dead assignment                                         scan_engine.cc          5872
> > 5872  res = recvtime(sd, recvbuf, 2048, 10, NULL);
> >       Value stored to 'res' is never read
> >       Is a Bug.  remove "res =" or do something with res.
>
> These are the kinds of bugs we're looking for. You should handle this
> case the same as other assignments to res in the same function.

The real problem is  "Should res be checked later and is not?"

> > Dead store    Dead assignment                                         traceroute.cc           1051
> > 1051  hop = host->insert_hop(ttl, from_addr, rtt);
> >       Value stored to 'hop' is never read
> >       Bug.  Hop is calculated, but never added to chain.  Misleading braket spacing.
>
> What is misleading about the spacing? I don't see it.

1048 if (hop == NULL) {
1051    hop = host->insert_hop(ttl, from_addr, rtt);
1052  } else {
1053    /* An existing hop at this address and TTL. Link this host's
chain to it. */
1054    if (o.debugging > 1) {
1055              log_write(LOG_STDOUT, "Traceroute cache hit %s TTL %d
while tracing",
1056               ss_to_string(&hop->addr), hop->ttl);
1057               log_write(LOG_STDOUT, " %s TTL %d\n",
host->target->targetipstr(), ttl);
1058        }
1059    
1060     host->link_to(hop);
1061    

     [snip the rest of the function]

1086    } //else
1087} //end function

Returns without doing anything with the hop variable that was set.
The } following the else looks like part of the else, but it is part
of the if that follows the else.  The rest of the function is working
with hop.

If that is correct, then we shouldn't set the local hop variable there.

> > Dead store    Dead increment                                          utils.cc                452
> > 452   bp += Snprintf(buf+bp, buflen-bp,"\n");
> >       Value stored to 'bp' is never read
> >       Bug.  Don't need this last assignment to bp.
>
> I think it would be a mistake to change this one too. Everywhere in the
> function is written "bp += Snprintf"; that is, write to the string and
> update the pointer. We shouldn't break this pattern just because bp
> isn't used after the last assignment.

I can see that.

> David Fifield
_______________________________________________
Sent through the nmap-dev mailing list
http://cgi.insecure.org/mailman/listinfo/nmap-dev
Archived at http://seclists.org/nmap-dev/