Re: Analysis of clang results for nmap main directory.

[David Fifield <david () bamsoftware com>]

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.

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

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

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

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