httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Nick Kew <n...@webthing.com>
Subject Re: svn commit: r790205 - /httpd/httpd/trunk/modules/experimental/mod_noloris.c
Date Wed, 01 Jul 2009 20:58:51 GMT
Ruediger Pluem wrote:

>> + * The major difference is that mod_antiloris checks the scoreboard
>> + * on every request.  This implies a per-request overhead that grows
>> + * with the scoreboard, and gets very expensive on a big server.
>> + * On the other hand, this module (mod_noloris) may be slower to
>> + * react to a DoS attack, and in the case of a very small server
>> + * it might be too late.
> 
> I am not sure if doing this for each connection (not each *request*)
> is really that much of a performace hit. I used a modified version of
> mod_limitipcon and do just that. So far I haven't noticed any performance
> issues with this approach. But maybe with a maximum of 1040 clients
> I am just having a small server :-).

I'm also thinking of simpler versions that set per-process limits
(no use of scoreboard/shm).  Obviously for servers with high numbers
of threads-per-server.  As and when round tuits permit.

>> +        apr_socket_close(csd);
> 
> This looks like an ugly thing to do for an module. Why not running the whole
> thing in the pre_connection hook and return something different then OK or DONE.
> This lets the core perform this dirty work in the predefined way.

I did have a good reason for process_connection over pre_connection,
but I can't bring it to mind right now.

>> +    /* store this client IP for the monitor to pick up */
>> +    /* under traditional scoreboard, none of this happens until
>> +     * there's a request_rec.  This is where we use the illegally-
>> +     * obtained private info from the scoreboard.
>> +     */
>> +
>> +    ws = &ap_scoreboard_image->servers[sbh->child_num][sbh->thread_num];
> 
> Shouldn't we use ap_get_scoreboard_worker here instead?

Probably, but it does nothing about the badness of how we got
child_num and thread_num.

>> +        ip = apr_palloc(pool, 18);
> 
> Harcoded values are IMHO ugly. Why 18 at all?

Erm .. 'cos I can't count.  Needs to hold the max size of
an (IPv6) address string.

>> +    memset(shm_rec, NULL, shm_size);
> 
> Why NULL and not 0?

Erm ... 'cos we have a heatwave, and I'm in an office in an
attic apartment with a south-facing window and low ceiling.

> Storing the configuration in global variables looks kind of ugly.
> But I get the point that it is hard to get them in the monitor hook.
> But at least a struct for them and a global pointer to the struct would
> make them look a little nicer :-).

Feel free to change it.  I don't really have any strong preference
there, and with GLOBAL_ONLY there's no context or hierarchy to
worry about.

Since it's now in svn, it's open to hack on


Mime
View raw message