You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Yann Ylavic <yl...@gmail.com> on 2014/12/01 14:15:18 UTC
Re: svn commit: r1640495 - /httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
On Wed, Nov 19, 2014 at 8:19 AM, <jk...@apache.org> wrote:
> Author: jkaluza
> Date: Wed Nov 19 07:19:13 2014
> New Revision: 1640495
>
> URL: http://svn.apache.org/r1640495
> Log:
> * mod_proxy_fcgi: Ignore body data from backend for 304 responses. PR 57198.
>
> Modified:
> httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
>
[]
> --- httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c (original)
> +++ httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c Wed Nov 19 07:19:13 2014
> @@ -369,7 +369,7 @@ static apr_status_t dispatch(proxy_conn_
> const char **err)
> {
> apr_bucket_brigade *ib, *ob;
> - int seen_end_of_headers = 0, done = 0;
> + int seen_end_of_headers = 0, done = 0, ignore_body = 0;
> apr_status_t rv = APR_SUCCESS;
> int script_error_status = HTTP_OK;
> conn_rec *c = r->connection;
> @@ -579,9 +579,16 @@ recv_again:
> APR_BRIGADE_INSERT_TAIL(ob, tmp_b);
> r->status = status;
> ap_pass_brigade(r->output_filters, ob);
> - ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01070)
> - "Error parsing script headers");
> - rv = APR_EINVAL;
> + if (status == HTTP_NOT_MODIFIED) {
> + /* The 304 response MUST NOT contain
> + * a message-body, ignore it. */
> + ignore_body = 1;
> + }
> + else {
> + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01070)
> + "Error parsing script headers");
> + rv = APR_EINVAL;
> + }
> break;
> }
>
There seems to be another case below this point where we "Send the
part of the body that we read while reading the headers" (as the
comment says), with no !ignore_body check.
Also, the backend may use a "Status: 304 Not Modified" header, which
would not result in ap_scan_script_header_err_brigade_ex() returning
that status but setting it to r->status.
So, more generally, shouldn't we check both
ap_scan_script_header_err_brigade_ex()'s returned status AND r->status
against 304 to ignore the body (ie. bypass ap_pass_brigade() for
anything but EOS)?
Regards,
Yann.
Re: svn commit: r1640495 - /httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
Posted by Jan Kaluža <jk...@redhat.com>.
On 12/01/2014 02:15 PM, Yann Ylavic wrote:
> On Wed, Nov 19, 2014 at 8:19 AM, <jk...@apache.org> wrote:
>> Author: jkaluza
>> Date: Wed Nov 19 07:19:13 2014
>> New Revision: 1640495
>>
>> URL: http://svn.apache.org/r1640495
>> Log:
>> * mod_proxy_fcgi: Ignore body data from backend for 304 responses. PR 57198.
>>
>> Modified:
>> httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
>>
> []
>> --- httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c (original)
>> +++ httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c Wed Nov 19 07:19:13 2014
>> @@ -369,7 +369,7 @@ static apr_status_t dispatch(proxy_conn_
>> const char **err)
>> {
>> apr_bucket_brigade *ib, *ob;
>> - int seen_end_of_headers = 0, done = 0;
>> + int seen_end_of_headers = 0, done = 0, ignore_body = 0;
>> apr_status_t rv = APR_SUCCESS;
>> int script_error_status = HTTP_OK;
>> conn_rec *c = r->connection;
>> @@ -579,9 +579,16 @@ recv_again:
>> APR_BRIGADE_INSERT_TAIL(ob, tmp_b);
>> r->status = status;
>> ap_pass_brigade(r->output_filters, ob);
>> - ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01070)
>> - "Error parsing script headers");
>> - rv = APR_EINVAL;
>> + if (status == HTTP_NOT_MODIFIED) {
>> + /* The 304 response MUST NOT contain
>> + * a message-body, ignore it. */
>> + ignore_body = 1;
>> + }
>> + else {
>> + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01070)
>> + "Error parsing script headers");
>> + rv = APR_EINVAL;
>> + }
>> break;
>> }
>>
>
> There seems to be another case below this point where we "Send the
> part of the body that we read while reading the headers" (as the
> comment says), with no !ignore_body check.
I agree with this part and I will fix it in my next commit.
> Also, the backend may use a "Status: 304 Not Modified" header, which
> would not result in ap_scan_script_header_err_brigade_ex() returning
> that status but setting it to r->status.
In this case, it's up to backend to not send the body and if it sends
some body together with 304 status, we don't have to care about that,
right? I can implement also check for r->status and set ignore_body=1,
but I thought it's not needed on httpd side.
> So, more generally, shouldn't we check both
> ap_scan_script_header_err_brigade_ex()'s returned status AND r->status
> against 304 to ignore the body (ie. bypass ap_pass_brigade() for
> anything but EOS)?
>
> Regards,
> Yann.
>
Regards,
Jan Kaluza
Re: svn commit: r1640495 - /httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Mon, Dec 1, 2014 at 2:15 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Wed, Nov 19, 2014 at 8:19 AM, <jk...@apache.org> wrote:
>> Author: jkaluza
>> Date: Wed Nov 19 07:19:13 2014
>> New Revision: 1640495
>>
>> URL: http://svn.apache.org/r1640495
>> Log:
>> * mod_proxy_fcgi: Ignore body data from backend for 304 responses. PR 57198.
>>
>> Modified:
>> httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
>>
> []
>> --- httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c (original)
>> +++ httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c Wed Nov 19 07:19:13 2014
>> @@ -369,7 +369,7 @@ static apr_status_t dispatch(proxy_conn_
>> const char **err)
>> {
>> apr_bucket_brigade *ib, *ob;
>> - int seen_end_of_headers = 0, done = 0;
>> + int seen_end_of_headers = 0, done = 0, ignore_body = 0;
>> apr_status_t rv = APR_SUCCESS;
>> int script_error_status = HTTP_OK;
>> conn_rec *c = r->connection;
>> @@ -579,9 +579,16 @@ recv_again:
>> APR_BRIGADE_INSERT_TAIL(ob, tmp_b);
>> r->status = status;
>> ap_pass_brigade(r->output_filters, ob);
>> - ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01070)
>> - "Error parsing script headers");
>> - rv = APR_EINVAL;
>> + if (status == HTTP_NOT_MODIFIED) {
>> + /* The 304 response MUST NOT contain
>> + * a message-body, ignore it. */
>> + ignore_body = 1;
>> + }
>> + else {
>> + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01070)
>> + "Error parsing script headers");
>> + rv = APR_EINVAL;
>> + }
>> break;
>> }
>>
>
> There seems to be another case below this point where we "Send the
> part of the body that we read while reading the headers" (as the
> comment says), with no !ignore_body check.
>
> Also, the backend may use a "Status: 304 Not Modified" header, which
> would not result in ap_scan_script_header_err_brigade_ex() returning
> that status but setting it to r->status.
> So, more generally, shouldn't we check both
> ap_scan_script_header_err_brigade_ex()'s returned status AND r->status
> against 304 to ignore the body (ie. bypass ap_pass_brigade() for
> anything but EOS)?
And maybe status 204 the same way too (no possible body with 204 either).
>
> Regards,
> Yann.