Home page logo
/

nmap-dev logo Nmap Development mailing list archives

Re: [Ncat] hang on ongoing ssl negotation in brokering mode
From: David Fifield <david () bamsoftware com>
Date: Tue, 21 Jun 2011 12:23:35 -0700

On Thu, Jun 16, 2011 at 11:51:52PM +0300, Shinnok wrote:
Hello David,

On 06/13/2011 11:09 PM, David Fifield wrote:

Please find a way to factor this code into its own function. It's too
much to have it repeated in each read loop.

            if(o.ssl && info->ssl && !info->ssl_accept_done){
                int ret = SSL_accept(info->ssl);
                int sslerr = 0;
...
                    goto loop_end;
                }
            }

Attached is a patch that tries to resolve this duplication issue,
however, I am not pleased with the outcome that much, since there still
is code duplication as well as unwarranted obfuscation as I guessed
previously.

However, maybe I am thinking it wrong, and I should just grab the entire
ssl negotiation logic code as is and paste it to some function that
takes as an argument not only an fdinfo structure(which is the clean
way), but the fd_list_t pointers so that I can take the necessary action
upon them as needed(rm_fd,etc..) directly from the function. However, I
would refrain from doing that, since it breaks modularity if I put that
kind of ncat_listen/ncat_broker specific logic in ncat_ssl.c.
There's also that fact that I would need to pass fds_ready too, and then
there is the question of how to skip the infinite loops(the continue's).
Oh, and the goto..

Having the same code copy-pasted in four places is not practically
maintainable. We need to find a better solution. The patch you attached
is better, even though it requires a duplicated switch block, in that
someone looking at that code will be led to the ssl_handshake function,
where they can make a change once.

Please commit this patch.

Maybe we are worrying too much about this code duplication issue, then
we should. The code in ncat_broker.c and ncat_listen.c is already
similar enough, especially in ncat_listen_stream vs. ncat_broker and
handle_connection in both. Passing stuff around and calling functions
adds to performance slowdown too, besides obfuscation, for no real gain.

You can't be serious about being concerned with function call overhead
here. That is vastly outweighed by the ongoing costs of code
maintenance, not to mention the runtime of the crypto operations
underneath.

The only other way, is to refactor the entire logic code in the
functions in question in order to make de-duplication possible, which is
probably not warranted at this time.

That is completely fine. That's what our test program is for. If you can
find a way to express the program flow better, please do it.

David Fifield
_______________________________________________
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 ]