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

[Daniel Miller <bonsaiviking () gmail com>]

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.


> 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.

In every use I could find, the "convenience" pointer was used 
exclusively, and the "dummy" port was not.

> Do you remember what specific part of the XML patch needed this nextPort
> change?
In addition to what I described above, handling destruction of a list of 
pointers to dynamically allocated structures became complicated. I 
started seeking a solution (which ended up being this patch) when I 
started seeing double-free bugs in valgrind.

> > 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.
> > 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?
Incredibly small: removal of a Port stack variable on certain function 
calls (148 bytes on my system)
> David Fifield
Dan
_______________________________________________
Sent through the nmap-dev mailing list
http://cgi.insecure.org/mailman/listinfo/nmap-dev
Archived at http://seclists.org/nmap-dev/