On Mon, Mar 12, 2018 at 9:39 AM, Rainer Jung <[email protected]> wrote: > Hi Yann, > > > Am 12.03.2018 um 13:24 schrieb [email protected]: >> >> 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.
