Nmap Development mailing list archives
Re: Analysis of clang results for nmap main directory.
From: Fyodor <fyodor () insecure org>
Date: Mon, 9 Jul 2012 14:58:30 -0700
On Mon, Jul 02, 2012 at 03:31:38PM -0400, James Rogers wrote:
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.
Thanks James. In some cases we like to keep the "useless" assignments because they ensure the proper state of variables that we use for accounting and such. In some cases we don't currently use them again in the function, but we still want them to have the correct values in case we ever change the function such that they are used again. Otherwise we end up with hard to debug bugs. But in other cases, the assignments are indeed superfluous. So I think most of your patch should be applied, but a few of the sections should be kept as in. Regarding the specifics, as we discussed in our meeting today: --- FPEngine.cc (revision 29046) +++ FPEngine.cc (working copy) @@ -2132,9 +2132,6 @@ if (o.debugging > 3 && this->timed_probes > 0) log_write(LOG_PLAIN, "[%s] Retransmitting timed probes (rcvd_before=%u, rcvd_now=%u times=%d).\n", this->target_host->targetipstr(), responses_stored, responses_now , this->fp_probes[0].getRetransmissions()); - /* Reset our local counters. */ - timed_probes_answered = 0; - timed_probes_timedout = 0; ^^^ We want to keep these for accounting purposes --- osscan.cc (revision 29046) +++ osscan.cc (working copy) @@ -540,7 +540,6 @@ /* error("Comp to %s: %li/%li=%f", o.reference_FPs1[i]->OS_name, num_subtests_succeeded, num_subtests, acc); */ if (acc >= FPR_entrance_requirement || acc == 1.0) { - state = 0; ^^^ Sure, please apply this one. --- osscan2.cc (revision 29046) +++ osscan2.cc (working copy) @@ -488,7 +488,6 @@ thisHostGood = HOS->hostSeqSendOK((*hostI)->hss, &tmptv); if (thisHostGood) { stime = tmptv; - foundgood = true; break; } @@ -660,7 +659,6 @@ thisHostGood = HOS->hostSendOK((*hostI)->hss, &tmptv); if (thisHostGood) { stime = tmptv; - foundgood = true; ^^^ Please apply the foundtrue part, but: @@ -2245,7 +2243,6 @@ if (!seq_gcd) { /* Constant ISN */ seq_rate = 0; - seq_stddev = 0; hss->si.index = 0; } else { ^^^ Let's keep this as it is, so our statistics are correct in case we ever do use them again. --- scan_engine.cc (revision 29046) +++ scan_engine.cc (working copy) @@ -4183,7 +4184,6 @@ fatal("Received -1 response from read_arp_reply_pcap"); if (rc == 0) { if (TIMEVAL_SUBTRACT(*stime, USI->now) < 0) { - timedout = true; break; ^^^ There are four cases of this. Let's keep the timedout state assignments as is so that the state variable is correct just in case we need to use it later. @@ -5868,7 +5865,7 @@ /* fuck, we are not aligned properly */ if (o.verbose || o.debugging) error("FTP command misalignment detected ... correcting."); - res = recvtime(sd, recvbuf, 2048, 10, NULL); + recvtime(sd, recvbuf, 2048, 10, NULL); ^^^ This looks good to apply. Normally we'd want to investigate whether we could/should do something with the return value rather than just not checking it, but since bounce scan has been working all these years without checking it, and since it is pretty much deprecated, let's just remove the assignment as your patch does. Also, as you note, it looks like we're just eating a buffer to try and get back into alignment here anyway. @@ -6010,7 +6007,7 @@ else scan[j].next = -1; } - current = pil.testinglist = &scan[0]; + pil.testinglist = &scan[0]; rsi.rpc_current_port = NULL; do { @@ -6212,7 +6209,7 @@ scan[j].next = j + 1; else scan[j].next = -1; } - current = pil.testinglist = &scan[0]; + pil.testinglist = &scan[0]; pil.firewalled = NULL; ^^^ These look good, but for safety, would you please add a NULL assignment to current and next back where they are declared? Here's the current line: struct portinfo *scan = NULL, *current, *next; --- traceroute.cc (revision 29046) +++ traceroute.cc (working copy) @@ -1048,7 +1048,7 @@ if (hop == NULL) { /* A new hop, never before seen with this address and TTL. Add it to the host's chain and to the global cache. */ - hop = host->insert_hop(ttl, from_addr, rtt); + host->insert_hop(ttl, from_addr, rtt); ^^^ Looks good, please apply. Cheers, Fyodor _______________________________________________ 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)