Re: [patch] Modify prototype for PortList::nextPort and get_port

[David Fifield <david () bamsoftware com>]

On Thu, May 24, 2012 at 04:03:36PM -0500, Daniel Miller wrote:
> List,
>
> I'm proposing a change with this patch that won't make any
> difference to users, but changes the way a couple functions are
> called to make them more straightforward for developers. Patch
> attached.
>
> I ran into this while working on my XML-structured-output patch,
> which needed some TLC with regard to memory management. Previously,
> calling PortList::nextPort required passing a Port object by
> reference, which would be modified with simple assignment to return
> the next port. The downside I ran into was that this prevents
> modifying Port objects with any heap-allocated structures without
> implementing a copy constructor, and that would be a lot of overhead
> for most calls, which discard a large number of Ports until the one
> desired is found. Fortunately, this return value was not used in any
> of the existing calls, since the function also returns a pointer to
> the Port object. It seemed straightforward to just trim out the
> parameter, saving the hassle and a small amount of stack memory from
> not needing to declare a Port object (several of which were labelled
> with comments declaring it a "dummy" object).

Can you elaborate on how the assignment was causing problems? The
assigment is designed to be a shallow copy; a copy constructor that
recursively made new copies of e.g. the service detection table would be
wrong.

You shouldn't ever pass a Port that has already been initialized and
points to dynamic memory to nextPort. The Port that you give to nextPort
is just a place to store the result. A pointer to that same structure is
also returned as a convenience. In other words, the uses where there's a
stack-allocated "dummy" port are correct.

Do you remember what specific part of the XML patch needed this nextPort
change?

> Unfortunately, it wasn't that easy: In the case of a port in the
> "default state" (e.g. filtered because of no-response for TCP),
> there wasn't a real Port object to point to. The solution I came up
> with is to modify the PortList::default_port_state Port object for
> each call and return a pointer to it. This has a few caveats, which
> I put in a comment in the function. Here's that section of the
> patch:

The whole reason for the complex interface is because of the default
port states. You can see how the interface changed right before adding
the default port states in an old nmap-mem branch:
svn log -v -r 16281:16283 https://svn.nmap.org/nmap-exp/david/nmap-mem@16417

I think that I added the requirement that the caller provide storage
specifically because of the default state portno issue you mentioned.
The other alternative, the one you've done in your patch, is to modify
one static copy of a default Port object. I'm not crazy about that
because, for example, it makes it unsafe to try to have two different
Port references at once.

> I realize it's a hard sell to change something that wasn't broken,
> but I thought that with the small memory improvement it was a useful
> enough change to separate it from the XML-structured-output patch.

What is the small memory improvement?

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