Re: svn commit: r1615289 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy_http.c

2014-08-25 Thread Rainer Jung 

Am 21.08.2014 um 14:57 schrieb Yann Ylavic:

On Sat, Aug 2, 2014 at 10:24 AM,  rj...@apache.org wrote:

Author: rjung
Date: Sat Aug  2 08:24:35 2014
New Revision: 1615289

URL: http://svn.apache.org/r1615289
Log:
PR53420: Proxy responses with error status and
ProxyErrorOverride On hang until proxy timeout.

Regression from 2.2. It was introduced by r912063
in order to fix PR41646.

Modified:
 httpd/httpd/trunk/CHANGES
 httpd/httpd/trunk/modules/proxy/mod_proxy_http.c


[...]

Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
URL: 
http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_http.c?rev=1615289r1=1615288r2=1615289view=diff
==
--- httpd/httpd/trunk/modules/proxy/mod_proxy_http.c (original)
+++ httpd/httpd/trunk/modules/proxy/mod_proxy_http.c Sat Aug  2 08:24:35 2014
@@ -1637,6 +1637,18 @@ int ap_proxy_http_process_response(apr_p
  if (!r-header_only  /* not HEAD request */
  (proxy_status != HTTP_NO_CONTENT)  /* not 204 */
  (proxy_status != HTTP_NOT_MODIFIED)) { /* not 304 */
+const char *tmp;
+/* Add minimal headers needed to allow http_in filter
+ * detecting end of body without waiting for a timeout. */
+if ((tmp = apr_table_get(r-headers_out, Content-Length))) {
+apr_table_set(backend-r-headers_in, Content-Length, 
tmp);
+}
+else if ((tmp = apr_table_get(r-headers_out, 
Transfer-Encoding))) {
+apr_table_set(backend-r-headers_in, Transfer-Encoding, 
tmp);
+}
+else if (te) {
+apr_table_set(backend-r-headers_in, Transfer-Encoding, 
te);
+}


Shouldn't these tests be in reverse order, ie. :
 if ((tmp = apr_table_get(r-headers_out, Transfer-Encoding))) {
 ...
 }
 else if ((tmp = apr_table_get(r-headers_out, Content-Length))) {
 ...
 }
 else if (te) {
 ...
 }
?

This is what http_in filter does, since according to the RFC, when
both TE and CL are there, the former wins.
I'm not sure it can happen because mod_proxy_http removes the CL above
in this case, but OTOH the code and comment below suggest
r-headers_out may change in between, so we probably want to be safe
about possible response splitting...


Thanks for the review Yann. Fixed in r1620324 and added to backport 
proposal.


Regards,

Rainer

Re: svn commit: r1615289 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy_http.c

2014-08-21 Thread Yann Ylavic 
On Sat, Aug 2, 2014 at 10:24 AM,  rj...@apache.org wrote:
 Author: rjung
 Date: Sat Aug  2 08:24:35 2014
 New Revision: 1615289

 URL: http://svn.apache.org/r1615289
 Log:
 PR53420: Proxy responses with error status and
 ProxyErrorOverride On hang until proxy timeout.

 Regression from 2.2. It was introduced by r912063
 in order to fix PR41646.

 Modified:
 httpd/httpd/trunk/CHANGES
 httpd/httpd/trunk/modules/proxy/mod_proxy_http.c

[...]
 Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
 URL: 
 http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_http.c?rev=1615289r1=1615288r2=1615289view=diff
 ==
 --- httpd/httpd/trunk/modules/proxy/mod_proxy_http.c (original)
 +++ httpd/httpd/trunk/modules/proxy/mod_proxy_http.c Sat Aug  2 08:24:35 2014
 @@ -1637,6 +1637,18 @@ int ap_proxy_http_process_response(apr_p
  if (!r-header_only  /* not HEAD request */
  (proxy_status != HTTP_NO_CONTENT)  /* not 204 */
  (proxy_status != HTTP_NOT_MODIFIED)) { /* not 304 */
 +const char *tmp;
 +/* Add minimal headers needed to allow http_in filter
 + * detecting end of body without waiting for a timeout. */
 +if ((tmp = apr_table_get(r-headers_out, Content-Length))) 
 {
 +apr_table_set(backend-r-headers_in, Content-Length, 
 tmp);
 +}
 +else if ((tmp = apr_table_get(r-headers_out, 
 Transfer-Encoding))) {
 +apr_table_set(backend-r-headers_in, 
 Transfer-Encoding, tmp);
 +}
 +else if (te) {
 +apr_table_set(backend-r-headers_in, 
 Transfer-Encoding, te);
 +}

Shouldn't these tests be in reverse order, ie. :
if ((tmp = apr_table_get(r-headers_out, Transfer-Encoding))) {
...
}
else if ((tmp = apr_table_get(r-headers_out, Content-Length))) {
...
}
else if (te) {
...
}
?

This is what http_in filter does, since according to the RFC, when
both TE and CL are there, the former wins.
I'm not sure it can happen because mod_proxy_http removes the CL above
in this case, but OTOH the code and comment below suggest
r-headers_out may change in between, so we probably want to be safe
about possible response splitting...

Regards,
Yann.