Re: ncat --max-conns

[Solar Designer <solar () openwall com>]

Disclaimer: I am replying out of context (I did not read the thread).
Yet I think my posting might be of help.

On Mon, Oct 05, 2009 at 09:56:59AM -0600, David Fifield wrote:
> It won't be safe to call logdebug from the handler function. It should
> be only
>
> static void handle_conn_count(void)
> {
>       conn_count--;
> }

Am I correct that there will be a "conn_count++;" somewhere else?  If
so, this is likely unreliable, because the "conn_count++;" will often be
non-atomic (it's read-modify-write).  Additionally, conn_count would
need to be declared "volatile sig_atomic_t", which is not great because
of the unknown range and maybe even signedness of this type (I am not
sure of the latter, though - maybe the signedness is in fact defined in
a standard).

To illustrate the issue with "conn_count++;" being non-atomic:

conn_count = N;
[...]
reg = conn_count; // read
reg++; // modify
// signal occurs here, does "conn_count--;"
conn_count = reg; // write

What's the value of conn_count?  It should probably be N (one "++", one
"--" since the moment we had N connections), but instead it stays at N+1
(as if the signal did not occur).  Over time, conn_count will get out of
sync with the reality.

Without "volatile" in the variable declaration, the write to conn_count
designated by "conn_count = reg;" above may be postponed for long - in
fact, it may even be done out of a certain loop (if the "conn_count++;"
was found in a loop).  Ditto for the read.

One way to deal with this would be to use two variables, like:

unsigned int conn_inc; // used in the main program only
volatile unsigned int conn_dec; // changed by the signal handler, read by the main program

Both would be incremented only (by the main program and by the signal
handler, respectively), and to determine the current connection count at
a given time, the main program would calculate "conn_inc - conn_dec".
Wraparounds are OK as long as these are same size and unsigned (the
latter is important: behavior on integer overflow is undefined for
signed types - the compiler is even free to omit the code, recent
versions of gcc do!)

Unfortunately, this is still not perfect, at least in theory.  The
problem is that reads of conn_dec by the main program might be
non-atomic - e.g., a 16-bit CPU might implement such reads of a 32-bit
variable with two separate instructions.  Perhaps this specific example
is irrelevant to platforms that Nmap might run on, and "unsigned int" is
unlikely to be wider than 32-bit on current platforms, but I am talking
about code correctness in general.

To solve this new problem, I think something like the following will
have to be used:

volatile sig_atomic_t conn_dec_changed;

static void handle_conn_count(void)
{
        conn_dec_changed = 1;
        conn_dec++;
}

static int get_conn_count(void)
{
        unsigned int conn_count;
        do {
                conn_dec_changed = 0;
                conn_count = conn_inc - conn_dec;
        } while (conn_dec_changed);
        if (conn_count > INT_MAX) return -1; // error, counts got out of sync
        return conn_count;
}

Alternatively:

static int get_conn_count(void)
{
        unsigned int conn_count;
        do {
                conn_dec_changed = 0;
                conn_count = conn_inc - conn_dec;
                if (conn_inc < conn_dec && !conn_dec_changed);
                        return -1; // error, counts got out of sync
        } while (conn_dec_changed);
        return conn_count;
}

The error checking is optional; it is to catch "higher-level" issues
(bugs elsewhere in the program or in other system software, lost or
spurious signals, etc.), not those I talked about above (I think that
the proposed approach deals with those fully).

-- 
Alexander Peslyak <solar at openwall.com>
GPG key ID: 5B341F15  fp: B3FB 63F4 D7A3 BCCC 6F6E  FC55 A2FC 027C 5B34 1F15
http://www.openwall.com - bringing security into open computing environments

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