Re: ncat --max-conns

[Solar Designer <solar () openwall com>]

David,

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.  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.  (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.)

You have a comment that says:

/* The number of connected clients is the difference of conn_inc and conn_dec.
   It is split up into two variables for signal safety. The number of connected
   clients must be modified through the functions increase_conn_count and
   decrease_conn_count, never not by modifying these variables directly.
   decrease_conn_count, called by the signal handler sigchld_handler, is the
   only place where conn_dec is written. As long as conn_dec is changing,
   conn_dec_changed is true. get_conn_count loops while conn_dec_changed is true
   to avoid data inconsistency when a read of conn_dec is interrupted by a
   signal. */

Notice how the comment is a bit inconsistent with the call to
decrease_conn_count() from the main program that I mentioned above.
Also, I see little or no need for having the increase_conn_count() and
decrease_conn_count() functions.  The emphasis should be not on going
via the functions, but rather on making the changes to conn_inc and
conn_dec from the correct places only.  With the current comment, it is
easy to overlook part of the original rationale for the split and to call
one of the functions from the wrong place ("I am using the functions, so
I must be OK" - wrong).

OK, I do see a reason for having decrease_conn_count() as a function
separate from the signal handler - you reuse that function in:

#ifdef WIN32
    set_pseudo_sigchld_handler(decrease_conn_count);
#else

...but that's the only reason.  I see no reason for having
increase_conn_count() as a separate function, especially not if you
actually need to do "conn_inc--;" as well (which is doubtful... wouldn't
SIGCHLD arrive anyway, thereby eventually resulting in the overall
connection count decreased by two? or is this code somehow only reached
in some non-forking mode? I am just guessing here, I did not "trace" the
code paths).

Of course, it is entirely possible that I overlooked something re: the
second issue.

Thanks again, and I hope that my quick review helps.

Alexander

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