On 10/21/2005 07:43 PM, William A. Rowe, Jr. wrote:
> Ruediger Pluem wrote:
>
> We might be better off using this fix (and documenting the usage of all get
> brigade calls w.r.t. transient buckets), while in 2.0.x we might want to
> return an allocated bucket in mod_ssl to ensure third party 2.0 modules,
> with
> this very same mis-assumptions, don't trip over this effect. Thoughts?
>
Just to avoid confusion about which patch we are talking. For trunk/2.2.x I will
commit the patch that exchanges APR_BRIGADE_CONCAT with ap_save_brigade.
If I understand you correctly you think that it might make more sense to use
Index: modules/ssl/ssl_engine_io.c
===================================================================
--- modules/ssl/ssl_engine_io.c»(Revision 326481)
+++ modules/ssl/ssl_engine_io.c»(Arbeitskopie)
@@ -1324,7 +1324,7 @@
/* Create a transient bucket out of the decrypted data. */
if (len > 0) {
apr_bucket *bucket =
- apr_bucket_transient_create(inctx->buffer, len,
f->c->bucket_alloc);
+ apr_bucket_heap_create(inctx->buffer, len, NULL,
f->c->bucket_alloc);
APR_BRIGADE_INSERT_TAIL(bb, bucket);
}
in the 2.0.x case to avoid third party modules falling in the same pit?
To be honest I would be -0.5 on that because
- The sideefects of aboves patch are currently unclear to me and we only apply
it to
the stable branch without having it in the trunk.
- As I stated, I think we face a typical setaside situation here. So it would
be the
duty of the third party module to fix this.
Nevertheless I agree that some third party modules might not give too much on
how they *should*
behave.
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/ssl/ssl_engine_io.c
===================================================================
--- modules/ssl/ssl_engine_io.c (Revision 326481)
+++ modules/ssl/ssl_engine_io.c (Arbeitskopie)
@@ -1324,7 +1324,7 @@
/* Create a transient bucket out of the decrypted data. */
if (len > 0) {
apr_bucket *bucket =
- apr_bucket_transient_create(inctx->buffer, len,
f->c->bucket_alloc);
+ apr_bucket_heap_create(inctx->buffer, len, NULL,
f->c->bucket_alloc);
APR_BRIGADE_INSERT_TAIL(bb, bucket);
}
