Re: Spurious port closed bug fix

[David Fifield <david () bamsoftware com>]

On Fri, Aug 17, 2012 at 09:21:40PM -0600, sean rivera wrote:
> I've written a very simple patch for the spurious port closed bug
> (below). All it does is create a local version of the o.magic_port
> variable and increments it by 256 every time we reach a new phase in
> the scan.

Thanks for this, Sean. Here are some comments inline.

> +/*I chose to declare this here in the same place as o in order to keep the code base as similar as possible*/
> +static u16 magic_port = o.magic_port;
> +

I think this initialization might have a problem known as the "static
initialization order fiasco." The problem is that the global o object
may be initialized before or after this assignment.

http://www.yosefk.com/c++fqa/ctors.html#fqa-10.12

I think what we should rather do is initialize this variable to a static
value not depending on another global object.

>    init_payloads(); /* Load up _all_ payloads into a mapped table */
> -
> +  if (magic_port > 65278) { //Catch overflow
> +    magic_port=33000; //Because we start out at 33000 everytime;
> +  }
> +  magic_port+=256;

The magic numnber 65278 needs some explanation. I see it is 2^16 - 258;
I guess that's because you want to leave at least tryno_cap + 256
headroom? How about just capping it at 65536 - 256? It should be written
that way in the source code.

Here's what I suggest for implementing this fix. Let's rename the
file-scope variable magic_port to base_port so there's no ore confusion
with the global o.magic_port. Remove this initialization from NmapOps.cc:
  magic_port = 33000 + (get_random_uint() % 31000);
and instead do this to base_port on the first call to ultra_scan. You
can use a boolean variable or a distinguished value in the static
initialization. The rest of the patch should work as you wrote it.

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