Nmap Development mailing list archives
[patch] Modify prototype for PortList::nextPort and get_port
From: Daniel Miller <bonsaiviking () gmail com>
Date: Thu, 24 May 2012 16:03:36 -0500
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).
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:
@@ -627,7 +627,7 @@ int PortList::getStateCounts(int state) const { function returns ports in numeric order from lowest to highest, except that if you ask for both TCP, UDP & SCTP, every TCP port will be returned before we start returning UDP and SCTP ports */ -Port *PortList::nextPort(const Port *cur, Port *next, +const Port *PortList::nextPort(const Port *cur, int allowed_protocol, int allowed_state) { int proto; int mapped_pno;@@ -653,13 +653,17 @@ Port *PortList::nextPort(const Port *cur, Port *next,for(;mapped_pno < port_list_count[proto]; mapped_pno++) { port = port_list[proto][mapped_pno]; if (port && (allowed_state==0 || port->state==allowed_state)) { - *next = *port; - return next; + return port; }if (!port && (allowed_state==0 || default_port_state[proto].state==allowed_state)) {- *next = default_port_state[proto]; - next->portno = port_map_rev[proto][mapped_pno]; - return next;+ /* Return pointer to default_port_state[proto] after setting portno. + * There is no reason anything should need the portno of the default state, which + * was previously 0. This can be thought of like std::string::c_str, in that + * data at the returned pointer may change upon further interaction with the+ * PortList object. This is probably not thread-safe. + */+ default_port_state[proto].portno = port_map_rev[proto][mapped_pno];+ return &default_port_state[proto]; } } }
In order to make sure no one modifies the default_port_state for a given PortList, I changed the prototype for nextPort to return a const Port *. This didn't cause any problems with any existing code in my tests and in my examination of the source. Finally, the get_ports function in nse_utility.cc had to be modified in the same ways (const return, drop the Port * parameter).
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.
Dan
Attachment:
nextPort.patch
Description:
_______________________________________________ Sent through the nmap-dev mailing list http://cgi.insecure.org/mailman/listinfo/nmap-dev Archived at http://seclists.org/nmap-dev/
Current thread:
- [patch] Modify prototype for PortList::nextPort and get_port Daniel Miller (May 24)
- Re: [patch] Modify prototype for PortList::nextPort and get_port David Fifield (Jun 13)
- Re: [patch] Modify prototype for PortList::nextPort and get_port Daniel Miller (Jun 14)
- Re: [patch] Modify prototype for PortList::nextPort and get_port David Fifield (Jun 14)
- Re: [patch] Modify prototype for PortList::nextPort and get_port Daniel Miller (Jun 14)
- Re: [patch] Modify prototype for PortList::nextPort and get_port Daniel Miller (Jun 14)
- Re: [patch] Modify prototype for PortList::nextPort and get_port Daniel Miller (Jun 14)
- Re: [patch] Modify prototype for PortList::nextPort and get_port David Fifield (Jun 13)