Meanwhile I identified 3 others positions in the proxy code path which can
cause this kind of trouble.
Please find the updated patches attached. Using APR_BRIGADE_CONCAT in a loop
with ap_get_brigade
on the same brigade seems to be troublesome :-).
Regards
Rüdiger
On 10/19/2005 11:10 AM, Ruediger Pluem wrote:
> Attached my (currently) final versions of the patches to fix PR37145 on 2.0.x
> (37145_2.0.x.diff)
> and on trunk (37145.diff). Comments / thoughts / votes are highly appreciated
> as I want to
> commit to trunk and propose it for backport in 2.0.x.
>
> Regards
>
> Rüdiger
>
> On 10/19/2005 02:46 AM, Ruediger Pluem wrote:
>
>>Attached a new version of the patch that uses ap_save_brigade.
>>Again for 2.0.x.
>>
>>Regards
>>
>>Rüdiger
>>
>>
>>On 10/19/2005 02:18 AM, Ruediger Pluem wrote:
>>
>>
>>>The following patch should fix PR37145. I would like to hear some comments /
>>>thoughts
>>
>>>from brigade / buckets and proxy gurus on this.
>>
>>>Although this problem has been reported against 2.0.55, I cross checked
>>>and this problem is also in the trunk.
>>>
>>>Regards
>>>
>>>Rüdiger
>>>
Index: modules/proxy/mod_proxy_http.c
===================================================================
--- modules/proxy/mod_proxy_http.c (Revision 326481)
+++ modules/proxy/mod_proxy_http.c (Arbeitskopie)
@@ -247,7 +247,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 {
@@ -354,7 +368,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 {
@@ -476,7 +504,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;
@@ -832,9 +874,27 @@
}
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 %pI (%s) from %s (%s)",
+ p_conn->addr, p_conn->hostname ? p_conn->hostname: "",
+ 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,
* surrender once we hit 80 bytes less than MAX_MEM_SPOOL
Index: modules/proxy/proxy_http.c
===================================================================
--- modules/proxy/proxy_http.c (Revision 326480)
+++ modules/proxy/proxy_http.c (Arbeitskopie)
@@ -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,9 +1123,27 @@
}
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,
* surrender once we hit 80 bytes less than MAX_MEM_SPOOL
