mailing list archives
Re: [patch] Modify prototype for PortList::nextPort and get_port
From: Daniel Miller <bonsaiviking () gmail com>
Date: Thu, 14 Jun 2012 11:56:32 -0500
On 06/13/2012 06:04 PM, David Fifield wrote:
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.
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
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.
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.
Do you remember what specific part of the XML patch needed this nextPort
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.
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
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.
Incredibly small: removal of a Port stack variable on certain function
calls (148 bytes on my system)
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?
Sent through the nmap-dev mailing list
Archived at http://seclists.org/nmap-dev/