Home page logo

nmap-dev logo Nmap Development mailing list archives

Re: Analysis of clang results for nmap main directory.
From: David Fifield <david () bamsoftware com>
Date: Thu, 14 Jun 2012 10:42:19 -0700

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
Archived at http://seclists.org/nmap-dev/

  By Date           By Thread  

Current thread:
[ Nmap | Sec Tools | Mailing Lists | Site News | About/Contact | Advertising | Privacy ]