Re: [PATCH] scan_engine.cc get_(ping_)?pcap_result() goodseq cleanup

[David Fifield <david () bamsoftware com>]

On Mon, Feb 09, 2009 at 12:11:53AM +0100, Daniel Roethlisberger wrote:
> I am still failing to fully understand all the nitty-gritty
> details of some of the code in scan_engine.cc.  There is a lot of
> copy-pasted duplicate code, sometimes with minor differences
> between the various appearances of the same code...
>
> Would the attached patch break anything?
>
> - Remove instances of `goodseq' which appear to be now defunct
>   remainders of previous trynum/pingseq decoding code (cf. TCP
>   sections of the functions).
> - Move the declarations of `goodseq' into the only context (code
>   block) which actually uses it.  This makes the code slightly
>   easier to understand, since it makes it obvious that `goodseq'
>   is only of local interest.

I applied these two parts of the patch. Cleanups are always welcome.

> - Add decoding and verification of trynum/pingseq to UDP
>   response handling (if no magic port was set).

This looks like the right thing to do too, but experience has taught me
always to test even small changes to scan_engine.cc. Can you do some
tests to ensure that the change doesn't affect speed or accuracy
negatively? I'm thinking of tests like

nmap -F -sU scanme.nmap.org
nmap -p1-65535 -sU localhost
nmap -F -sU -iL up-hosts

where up-hosts is a dozen hosts with responsive UDP ports, found with
nmap -n -sP -PU -iR 500. For each test, record the time taken and the
number of ports in each state.

> +        /* Replace this with a call to probe_check_trynum_pingseq or similar. */
> +        if (o.magic_port_set) {
> +          trynum = probe->tryno;
> +          pingseq = probe->pingseq;
> +        } else {
> +          sport_decode(USI, o.magic_port, ntohs(udp->uh_dport), &trynum, &pingseq);
> +        }
> +
> +        /* Make sure that trynum and pingseq match the values in the probe. */
> +        if (!probe->check_tryno_pingseq(trynum, pingseq))
> +          continue;
> +

There's another place in the code with "Replace this with a call...",
but it doesn't set pingseq if o.magic_port_set and it's not followed by
the "Make sure that..." block. If this change is beneficial, do you
think the code should be the same in both places, or is there a reason
for the difference?

David Fifield

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