On Mon, Mar 12, 2018 at 9:39 AM, Rainer Jung <rainer.j...@kippdata.de> wrote:
> Hi Yann,
>
>
> Am 12.03.2018 um 13:24 schrieb yla...@apache.org:
>>
>> Author: ylavic
>> Date: Mon Mar 12 12:24:27 2018
>> New Revision: 1826543
>>
>> URL: http://svn.apache.org/viewvc?rev=1826543&view=rev
>> Log:
>> Fix timeout logging in ap_process_request().
>>
>> We can't use 'r' after ap_process_request_after_handler(), the core output
>> filter might have cleaned up its deferred bucket brigade on error,
>> including
>> the EOR bucket.
>>
>> Reported by: steffenal
>> Fixes SpiderLabs/ModSecurity#1542
>>
>>
>> Modified:
>>      httpd/httpd/branches/2.4.x/modules/http/http_request.c
>>
>> Modified: httpd/httpd/branches/2.4.x/modules/http/http_request.c
>> URL:
>> http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/http/http_request.c?rev=1826543&r1=1826542&r2=1826543&view=diff
>>
>> ==============================================================================
>> --- httpd/httpd/branches/2.4.x/modules/http/http_request.c (original)
>> +++ httpd/httpd/branches/2.4.x/modules/http/http_request.c Mon Mar 12
>> 12:24:27 2018
>> @@ -480,13 +480,9 @@ AP_DECLARE(void) ap_process_request(requ
>>                * Notice a timeout as an error message. This might be
>>                * valuable for detecting clients with broken network
>>                * connections or possible DoS attacks.
>> -             *
>> -             * It is still safe to use r / r->pool here as the eor bucket
>> -             * could not have been destroyed in the event of a timeout.
>>                */
>> -            ap_log_rerror(APLOG_MARK, APLOG_INFO, rv, r, APLOGNO(01581)
>> -                          "Timeout while writing data for URI %s to the"
>> -                          " client", r->unparsed_uri);
>> +            ap_log_cerror(APLOG_MARK, APLOG_INFO, rv, c, APLOGNO(01581)
>> +                          "flushing data to the client");
>>           }
>>       }
>>       if (ap_extended_status) {
>
>
> was it intentional to apply this directly to 2.4 without trunk (don't know
> whether it applies there) and STATUS? Maybe because it is a logging only
> change?
>
> And was it intentional to change the contents of the log message, not only
> cerror/c instead of rerror/r? If the change in log message text was
> intentional, shouldn't we then use a new APLOGNO? And while "Timeout" sounds
> like INFO or higher, "flushing" sounds more like a DEBUG or lower, at least
> to me.

I had the same thought re: the content change, but I guess the timeout
part is redundant due to the rv we will pass in.   I didn't look in
context for write vs flush but i assume it is more accurate (just
confidence in Yann)

All things being equal I think keeping the ID is probably the best
compromise -- since it's the same condition.

Reply via email to