Re: ncat should try connecting to all resolved addresses, not only the first one

[David Fifield <david () bamsoftware com>]

On Thu, Apr 17, 2014 at 05:27:47AM -0400, Jaromir Koncicky wrote:
> I'm reminding again because there was no response during almost two
> months.
> I'd really like to know what is the state of my patch, whether
> anything is blocking it from being applied and needs to be done or it
> can just be applied.
> I got some comments from David which I reacted on in
> http://seclists.org/nmap-dev/2014/q1/100 and attached a slightly
> improved patch, but this is most likely the best I can do. 
> If it's still not good enough then I'd appreciate some help.

Thanks for following up on this patch. The patch in
http://seclists.org/nmap-dev/2014/q1/100 is not too bad. I'm not fully
happy with it, but it looks good enough to be merged.

I still think it is a major mistake to use a linked list where the first
element is statically allocated and the others are dynamically
allocated. It's a confusing design that is just begging for difficult
debugging in the future. You wrote "you don't need to dynamically
allocate memory in case that only one address is resolved," but that's a
big disadvantage, not an advantage. If there's an error in the code that
runs only for n > 1 targets, it will cause a problem only sometimes and
be hard to diagnose. If the same code always runs whether n == 1 or n > 1,
then any problem is easy to diagnose.

I take your point that the current code uses AF_UNSPEC as a signal for a
missing target. If it's truly easier to work with that way, then it
should be explicit: use one variable for the first target, and a
dynamically allocated list for all the others. Or, just enforce that the
target list always contain at least one element, set to AF_UNSPEC if
there is no target. However changing existing tests for AF_UNSPEC to be
for NULL makes more sense to me, at first glance.

I don't like the change to add a boolean flag to resolve_internal. I
think that basic functions like that should be basic, and not do a lot
of things optionally. If a boolean flag amounts to having two different
functions (one for true and one for false), then there should be two
different functions. If those functions share a lot of code, it should
be factored out into a third function with local scope.

add_nsock_connection would be better named as try_nsock_connect or
something. "add_nsock_connection" makes me think it's building some list
of addresses to connect to, when in reality the list is already
constructed (in targetaddrs) and it's going to try the next thing on the
list.

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