Re: Analysis of clang results for nmap main directory.

[David Fifield <david () bamsoftware com>]

On Thu, Jun 14, 2012 at 02:03:32PM -0400, James Rogers wrote:
> 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.

I don't think that's right. Did you try it? I think it would cause
output like

Service Info:; OS: Unix

instead of the expected

Service Info: OS: Unix

> > > 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?"

It should be checked immediately after the recvtime, and is not. Look
elsewhere in the function; recvtime errors are already being handled;
this looks like a simple oversight.

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

I think the indentation reflects the program flow exactly. The "else"
part of the branch is using the hop from hop_cache_lookup, not
host->insert_hop. You can remove the hop assignment in the "hop == NULL"
branch. Maybe the function originally returned hop or something, in
which case we would need the assignment there.

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