Nmap Development mailing list archives
Re: Analysis of clang results for nmap main directory.
From: James Rogers <jamesmrogers () gmail com>
Date: Mon, 9 Jul 2012 10:21:28 -0700
Bump for comment on whether or not this change set is acceptable. On Mon, Jul 2, 2012 at 12:31 PM, James Rogers <jamesmrogers () gmail com> wrote:
Here is a diff for the clang changes we talked about. On Thu, Jun 14, 2012 at 2:12 PM, David Fifield <david () bamsoftware com> wrote: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: UnixDead 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/
Current thread:
- Re: Analysis of clang results for nmap main directory. James Rogers (Jul 02)
- Re: Analysis of clang results for nmap main directory. James Rogers (Jul 09)
- Re: Analysis of clang results for nmap main directory. Fyodor (Jul 09)