Re: Analysis of clang results for nmap main directory.

[Fyodor <fyodor () insecure org>]

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/