Home page logo
/

nmap-dev logo 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/

  By Date           By Thread  

Current thread:
[ Nmap | Sec Tools | Mailing Lists | Site News | About/Contact | Advertising | Privacy ]