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

[Daniel Miller <bonsaiviking () gmail com>]

On 06/14/2012 12:18 PM, David Fifield wrote:
> On Thu, Jun 14, 2012 at 11:56:32AM -0500, Daniel Miller wrote:
> > On 06/13/2012 06:04 PM, David Fifield wrote:
> > > On Thu, May 24, 2012 at 04:03:36PM -0500, Daniel Miller wrote:
> > > > The downside I ran into was that this prevents
> > > > modifying Port objects with any heap-allocated structures without
> > > > implementing a copy constructor,
> > > 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.
> > The problem was that if a copy constructor is not explicitly
> > declared, a naive one is created that simply does a shallow copy.
> > The Port::scriptResults member variable is a ScriptResults type, a
> > typedef'd std::list<ScriptResult>. This works just fine for the
> > current situation, but since my patch involves dynamically allocated
> > data structures as part of the ScriptResult class, I changed the
> > ScriptResults typedef to std::list<ScriptResult *>.
> > std::list::push_back() (as used in e.g. PortList::addScriptResult())
> > makes a copy of whatever is being pushed onto the list. Because my
> > version of the ScriptResult class involves dynamic memory, I started
> > passing around pointers instead of implementing copy constructors.
> Whatever new dynamically allocated data structures you use, treat them
> the same way as the existing dynamically allocated structures. Don't
> deallocate memory in a destructor. See the freeService member and the
> commit that introduced it:
>
> svn log -r 16311 https://svn.nmap.org/nmap-exp/david/nmap-mem@16417
> svn diff -c 16311 https://svn.nmap.org/nmap-exp/david/nmap-mem@16417
>
> A shallow copy (i.e., the memcpy that the compiler will generate) is
> what we want here. A Port is not a standalone object; rather it is a
> kind of reference into PortList::port_list; individual Port object
> should not have an existence (like their own destructor) apart from
> their PortList. This is also why you have to use PortList::createPort
> instead of just doing "new Port()". It is complicated and confusing,
> only justified by the huge memory savings it gives.
>
> > > > 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 do see this as the strongest argument against my method as it
> > stands. One thing I hadn't considered (and will have to try out) is
> > treating the Port::scriptResults member like the Port::service
> > member. As I understand it, it's a pointer to a dynamically
> > allocated data structure, so that as long as it is not allocated
> > (pointer is NULL), a copy can be made without issue. It brings us
> > back around to explicit memory management, though, since as I found,
> > freeing memory in a destructor means you can't make shallow copies
> > or you end up with double-frees.
> Don't use a Port destructor; find another way to do what you're trying
> to do. This nextPort change won't be merged, so please redo your
> structured XML not to use it.
>
> You might have more joy just using a lua_State and a Lua table to
> represent the script output, rather than reconstructing an equivalent
> structure (ScriptOutputNode).
>
> David Fifield
This turned out to be easier than I thought. I've updated my git repo at 
https://github.com/bonsaiviking/Nmap/tree/xml-output if you want to see 
how it looks currently. As we continue to refine the requirements, I may 
look into the lua_State idea.

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