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

[Daniel Miller <bonsaiviking () gmail com>]

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/