Re: ncat --max-conns

[David Fifield <david () bamsoftware com>]

On Fri, Oct 09, 2009 at 11:38:24PM +0400, Solar Designer wrote:
> On Fri, Oct 09, 2009 at 12:49:37PM -0600, David Fifield wrote:
> > I've added your proposed code to Ncat. Thanks for your help.
>
> Thank you!
>
> I've checked out the latest SVN code, and you seem to have a couple of
> bugs in the way you've implemented my proposed approach:
>
> static volatile sig_atomic_t conn_dec_changed = 0;
> static unsigned int conn_inc = 0;
> static unsigned int conn_dec = 0;
>
> conn_dec must be declared "volatile" too, otherwise it may be pre-loaded
> into a register before the loop in get_conn_count().  I did specify it
> as "volatile" in my posting, although I admit I did not explain that
> this was critical even with the approach I proposed further down that
> posting as well.  Also, conn_dec_changed is always initialized before
> being read, so there's no need to pre-initialize it (not to mention that
> all global variables are in initialized-to-zero sections anyway; I do
> agree that explicit initialization to zero where needed may help clarify
> things).  So I propose:
>
> static unsigned int conn_inc = 0;
> static volatile unsigned int conn_dec = 0;
> static volatile sig_atomic_t conn_dec_changed;
>
> Then, you're calling decrease_conn_count() not only from the signal
> handler, but also from read_socket(), which I think is called from the
> main program.  This defeats an assumption I had made (and mentioned in
> my posting) - namely, that conn_dec is only changed by a singal handler
> (and the signal is automatically blocked for the duration of the signal
> handler run, thereby preventing re-entry), whereas conn_inc is only
> changed by the main program.

Ah, I see now. I misunderstood--I had thought the important thing ws
that conn_dec_changed be set when conn_dec was changed.

> When the same variable (conn_dec) may be
> changed by both the main program and the signal handler, we're back to
> where we started. :-(  Luckily, I think the fix is as trivial as
> replacing this call to decrease_conn_count() from the main program with
> "conn_inc--;" - changing only the variable that is OK to change in the
> main program.

All right, that's a good idea and I have done it.

> (BTW, I doubt that it is correct to decrease the connection count in
> that place.  This logic might be flawed.  But I am only speaking of
> the races I pointed out initially, still staying somewhat out of
> context of the Ncat specific discussion.)

There are two ways for the connection count to decrease, depending on
whether the --exec (or --sh-exec) option was used. With --exec, the
count is decreased when a child process ends. Without --exec, it is
decreased when the TCP connection ends.

Thank you again. I ahve done my best to effect these changes in r15786.
I welcome your review of it.

David Fifield

_______________________________________________
Sent through the nmap-dev mailing list
http://cgi.insecure.org/mailman/listinfo/nmap-dev
Archived at http://SecLists.Org