You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Rainer Jung <ra...@kippdata.de> on 2018/03/12 13:39:04 UTC
Re: svn commit: r1826543 -
/httpd/httpd/branches/2.4.x/modules/http/http_request.c
Hi Yann,
Am 12.03.2018 um 13:24 schrieb ylavic@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.
Regards,
Rainer
Re: svn commit: r1826543 - /httpd/httpd/branches/2.4.x/modules/http/http_request.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Mon, Mar 12, 2018 at 3:17 PM, Eric Covener <co...@gmail.com> wrote:
> On Mon, Mar 12, 2018 at 9:39 AM, Rainer Jung <ra...@kippdata.de> wrote:
>>
>> Am 12.03.2018 um 13:24 schrieb ylavic@apache.org:
>>>
>>> ==============================================================================
>>> --- 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?
Oops, really not at all :/
Will revert there and re-commit in trunk, thanks for noticing.
>>
>> 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.
Actually I don't know the "rule" to change (or not) an APLOGNO when a
message is modified...
>
> 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)
Yes, strerror(APR_TIMEUP) mentions the timeout already, so while at it
I took the liberty to simplify the message.
We are flushing outgoing data before (possibly) blocking for incoming
ones here, and "flushing data to the client" is aligned to the
core_output_filter's logging style (since it's now connection level I
thought it made sense).
>
> All things being equal I think keeping the ID is probably the best
> compromise -- since it's the same condition.
My opinion too...
Re: svn commit: r1826543 - /httpd/httpd/branches/2.4.x/modules/http/http_request.c
Posted by Eric Covener <co...@gmail.com>.
On Mon, Mar 12, 2018 at 9:39 AM, Rainer Jung <ra...@kippdata.de> wrote:
> Hi Yann,
>
>
> Am 12.03.2018 um 13:24 schrieb ylavic@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.