You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by co...@apache.org on 2006/01/24 23:57:40 UTC

svn commit: r372046 - in /httpd/httpd/branches/2.0.x: CHANGES STATUS modules/proxy/proxy_http.c

Author: colm
Date: Tue Jan 24 14:57:33 2006
New Revision: 372046

URL: http://svn.apache.org/viewcvs?rev=372046&view=rev
Log:
Merge r327590 from trunk:

* Fix PR37145 (data loss with httpd-2.0.55 reverse proxy method=post) by
  exchanging APR_BRIGADE_CONCAT with ap_save_brigade to ensure that
  transient buckets get setaside correctly between various iterations of
  ap_get_brigade calls.

Submitted by: rpluem

Modified:
    httpd/httpd/branches/2.0.x/CHANGES
    httpd/httpd/branches/2.0.x/STATUS
    httpd/httpd/branches/2.0.x/modules/proxy/proxy_http.c

Modified: httpd/httpd/branches/2.0.x/CHANGES
URL: http://svn.apache.org/viewcvs/httpd/httpd/branches/2.0.x/CHANGES?rev=372046&r1=372045&r2=372046&view=diff
==============================================================================
--- httpd/httpd/branches/2.0.x/CHANGES [utf-8] (original)
+++ httpd/httpd/branches/2.0.x/CHANGES [utf-8] Tue Jan 24 14:57:33 2006
@@ -1,6 +1,10 @@
                                                          -*- coding: utf-8 -*-
 Changes with Apache 2.0.56
 
+  *) mod_proxy_http: Prevent data corruption of POST request bodies when
+     client accesses proxied resources with SSL. PR 37145.
+     [Ruediger Pluem, William Rowe]    
+
   *) Elimiated the NET_TIME filter, restructuring the timeout logic.
      This provides a working mod_echo on all platforms, and ensures any
      custom protocol module is at least given an initial timeout value

Modified: httpd/httpd/branches/2.0.x/STATUS
URL: http://svn.apache.org/viewcvs/httpd/httpd/branches/2.0.x/STATUS?rev=372046&r1=372045&r2=372046&view=diff
==============================================================================
--- httpd/httpd/branches/2.0.x/STATUS (original)
+++ httpd/httpd/branches/2.0.x/STATUS Tue Jan 24 14:57:33 2006
@@ -106,15 +106,6 @@
 
 RELEASE SHOWSTOPPERS:
 
-   *) mod_proxy: Fix PR37145 (data loss with httpd-2.0.55 reverse proxy
-      method=post).
-      Ruediger says: This is a regression against 2.0.54 and causes
-                     severe trouble with reverse proxy configurations
-                     in conjunction with SSL. Many users have complained
-                     about this problem in bugzilla and on the dev list.
-                     Plus: It is only missing one vote :-)
-
-
 PATCHES ACCEPTED TO BACKPORT FROM TRUNK:
   [ start all new proposals below, under PATCHES PROPOSED. ]
 
@@ -146,14 +137,6 @@
          jim:    +1 on http://svn.apache.org/viewcvs?rev=326018&view=rev
                  as well.
          colm:   another +1 on the later patch too.
-
-    *) mod_proxy: Fix PR37145
-       (data loss with httpd-2.0.55 reverse proxy method=post).
-       Trunk version of patch:
-          http://svn.apache.org/viewcvs.cgi?rev=327590&view=rev
-       Backport version for 2.0.x of patch:
-          http://issues.apache.org/bugzilla/attachment.cgi?id=16744
-       +1: rpluem, trawick, jim
 
     *) mod_cache: Fix PR37347
        (mod_disk_cache replaces HTTP Status 301 with 200)

Modified: httpd/httpd/branches/2.0.x/modules/proxy/proxy_http.c
URL: http://svn.apache.org/viewcvs/httpd/httpd/branches/2.0.x/modules/proxy/proxy_http.c?rev=372046&r1=372045&r2=372046&view=diff
==============================================================================
--- httpd/httpd/branches/2.0.x/modules/proxy/proxy_http.c (original)
+++ httpd/httpd/branches/2.0.x/modules/proxy/proxy_http.c Tue Jan 24 14:57:33 2006
@@ -504,7 +504,21 @@
              * take care of that now
              */
             bb = header_brigade;
-            APR_BRIGADE_CONCAT(bb, input_brigade);
+
+            /*
+             * Save input_brigade in bb brigade. (At least) in the SSL case
+             * input_brigade contains transient buckets whose data would get
+             * overwritten during the next call of ap_get_brigade in the loop.
+             * ap_save_brigade ensures these buckets to be set aside.
+             * Calling ap_save_brigade with NULL as filter is OK, because
+             * bb brigade already has been created and does not need to get
+             * created by ap_save_brigade.
+             */
+            status = ap_save_brigade(NULL, &bb, &input_brigade, p);
+            if (status != APR_SUCCESS) {
+                return status;
+            }
+
             header_brigade = NULL;
         }
         else {
@@ -611,7 +625,21 @@
              * take care of that now
              */
             bb = header_brigade;
-            APR_BRIGADE_CONCAT(bb, input_brigade);
+
+            /*
+             * Save input_brigade in bb brigade. (At least) in the SSL case
+             * input_brigade contains transient buckets whose data would get
+             * overwritten during the next call of ap_get_brigade in the loop.
+             * ap_save_brigade ensures these buckets to be set aside.
+             * Calling ap_save_brigade with NULL as filter is OK, because
+             * bb brigade already has been created and does not need to get
+             * created by ap_save_brigade.
+             */
+            status = ap_save_brigade(NULL, &bb, &input_brigade, p);
+            if (status != APR_SUCCESS) {
+                return status;
+            }
+
             header_brigade = NULL;
         }
         else {
@@ -735,7 +763,21 @@
             apr_brigade_cleanup(input_brigade);
         }
         else {
-            APR_BRIGADE_CONCAT(body_brigade, input_brigade);
+
+            /*
+             * Save input_brigade in body_brigade. (At least) in the SSL case
+             * input_brigade contains transient buckets whose data would get
+             * overwritten during the next call of ap_get_brigade in the loop.
+             * ap_save_brigade ensures these buckets to be set aside.
+             * Calling ap_save_brigade with NULL as filter is OK, because
+             * body_brigade already has been created and does not need to get
+             * created by ap_save_brigade.
+             */
+            status = ap_save_brigade(NULL, &body_brigade, &input_brigade, p);
+            if (status != APR_SUCCESS) {
+                return status;
+            }
+
         }
         
         bytes_spooled += bytes;
@@ -1081,8 +1123,26 @@
         }
 
         apr_brigade_length(temp_brigade, 1, &bytes);
-        APR_BRIGADE_CONCAT(input_brigade, temp_brigade);
         bytes_read += bytes;
+
+        /*
+         * Save temp_brigade in input_brigade. (At least) in the SSL case
+         * temp_brigade contains transient buckets whose data would get
+         * overwritten during the next call of ap_get_brigade in the loop.
+         * ap_save_brigade ensures these buckets to be set aside.
+         * Calling ap_save_brigade with NULL as filter is OK, because
+         * input_brigade already has been created and does not need to get
+         * created by ap_save_brigade.
+         */
+        status = ap_save_brigade(NULL, &input_brigade, &temp_brigade, p);
+        if (status != APR_SUCCESS) {
+            ap_log_error(APLOG_MARK, APLOG_ERR, status, r->server,
+                         "proxy: processing prefetched request body failed"
+                         " to %s from %s (%s)",
+                         p_conn->name ? p_conn->name: "",
+                         c->remote_ip, c->remote_host ? c->remote_host: "");
+            return status;
+        }
 
     /* Ensure we don't hit a wall where we have a buffer too small
      * for ap_get_brigade's filters to fetch us another bucket,