You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ruediger Pluem <rp...@apache.org> on 2005/10/19 02:18:05 UTC
[PATCH] PR37145
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/proxy_http.c
===================================================================
--- modules/proxy/proxy_http.c (Revision 312938)
+++ modules/proxy/proxy_http.c (Arbeitskopie)
@@ -1080,6 +1080,18 @@
return status;
}
+ /*
+ * setaside the buckets of temp_brigade before concating input_brigade
+ * with temp_brigade. (At least) in the SSL case temp_brigade contains
+ * transient buckets whose data get overwritten during the next call
+ * of ap_get_brigade in the loop.
+ */
+ for (e = APR_BRIGADE_FIRST(temp_brigade);
+ e != APR_BRIGADE_SENTINEL(temp_brigade);
+ e = APR_BUCKET_NEXT(e)) {
+ apr_bucket_setaside(e, p);
+ }
+
apr_brigade_length(temp_brigade, 1, &bytes);
APR_BRIGADE_CONCAT(input_brigade, temp_brigade);
bytes_read += bytes;
Re: [PATCH] PR37145: data loss with httpd-2.0.55 reverse proxy method=post
Posted by Ruediger Pluem <rp...@apache.org>.
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
Re: [PATCH] PR37145: data loss with httpd-2.0.55 reverse proxy method=post
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Ruediger Pluem wrote:
>
> On 10/19/2005 08:25 PM, William A. Rowe, Jr. wrote:
>
> [..cut..]
>
>>Researching as well.
>
> Any new results from your research? Otherwise I would like to commit the latest version
> of my patch to trunk if you have no objections.
Yes ... and from the breadth of your other patch, and other non-BRIGADE_CONCAT
cases in the code, I'm taking a deeper look at the semantics of transient
buckets. As originally conceived, the contract was pretty clear, they existed
on the stack (or similar) and could be trusted only until the callee returned.
In this case, obviously, they exist until brigade get is called again. But
we had never spelled out how transient a transient bucket is, except in the
case of pushing output buckets down pass brigade.
Go ahead and commit, it's obviously an effective solution for this specific
set of cases. I'm not certain, however, that it's the entire fix :-/
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?
Bill
Re: [PATCH] PR37145: data loss with httpd-2.0.55 reverse proxy method=post
Posted by Ruediger Pluem <rp...@apache.org>.
On 10/19/2005 08:25 PM, William A. Rowe, Jr. wrote:
[..cut..]
> Researching as well.
>
Any new results from your research? Otherwise I would like to commit the latest version
of my patch to trunk if you have no objections.
Regards
Rüdiger
Re: [PATCH] PR37145: data loss with httpd-2.0.55 reverse proxy method=post
Posted by Jeff Trawick <tr...@gmail.com>.
On 10/19/05, Ruediger Pluem <rp...@apache.org> wrote:
>
>
> On 10/19/2005 10:44 PM, William A. Rowe, Jr. wrote:
>
> [..cut..]
>
> >
> > The problem is -not- in creating the transient buckets (if they are
> > sent, that's
> > goodness). The problem is in transforming them to persistant buckets
> > before the
> > core, ssl, or other filters who have set-aside operations decide to
> > return. So
> > there should have been no trouble creating the transient bucket below, the
> > trouble came in when the core filter didn't send the data, and also
> > didn't set
> > it aside :-(
>
> Sorry, maybe I am only confused, but I think I disagree with you on that.
> The proxy code is reading the input filter chain in a loop and does repeated
> calls to ap_get_brigade without doing any more things with these brigades
> it gets from ap_get_brigade, but storing them for later processing. This
> looks to me like a typical setaside situation.
> The data was not send because ap_proxy_http_request kept it for itself
> not sending it anyware.
+1
Re: [PATCH] PR37145: data loss with httpd-2.0.55 reverse proxy method=post
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Ruediger Pluem wrote:
>
> On 10/21/2005 04:06 PM, Joe Orton wrote:
>
> [..cut..]
>
>>I agree that's the correct analysis, your patch to fix the proxy to use
>>ap_save_brigade looks good to me.
>
> Thanks for feedback. I will commit later to give otherBill a chance for
> feedback.
> One technical question: As this bug was reported against 2.0.55 I would like
> to propose this patch for backport. As I am only committer am I allowed
>
> - to add it to the 2.0.x STATUS file
> - add my (of course non binding) vote on this backport?
Feel free to add my +1 - this is a solution to an obvious problem, even if this
is a semantic problem that we might decide, later, to clear up in mod_ssl.
Bill
Re: [PATCH] PR37145: data loss with httpd-2.0.55 reverse proxy method=post
Posted by Jeff Trawick <tr...@gmail.com>.
On 10/21/05, Ruediger Pluem <rp...@apache.org> wrote:
>
>
> On 10/21/2005 04:06 PM, Joe Orton wrote:
>
> [..cut..]
>
> > I agree that's the correct analysis, your patch to fix the proxy to use
> > ap_save_brigade looks good to me.
>
> Thanks for feedback. I will commit later to give otherBill a chance for
> feedback.
> One technical question: As this bug was reported against 2.0.55 I would like
> to propose this patch for backport. As I am only committer am I allowed
>
> - to add it to the 2.0.x STATUS file
yes
> - add my (of course non binding) vote on this backport?
yes; I cannot think of a reason why your vote would not be respected
in this particular matter
Re: [PATCH] PR37145: data loss with httpd-2.0.55 reverse proxy
method=post
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
--On October 21, 2005 11:34:47 PM +0200 Ruediger Pluem <rp...@apache.org>
wrote:
> Sorry, but I am confused. I thought only PMC members have binding votes.
> Or is my vote binding because I proposed the backport?
Since you have commit access to httpd, the intent is for you to be able to
vote on any changes to httpd. So, if you want to vote or veto something,
have at it.
In short, assume that your votes count until someone complains...and if
that person complains, then that person should feel obligated to nominate
you for PMC status because if they try to block you- it means that you're
actually doing something. =) -- justin
Re: [PATCH] PR37145: data loss with httpd-2.0.55 reverse proxy method=post
Posted by Ruediger Pluem <rp...@apache.org>.
On 10/21/2005 08:34 PM, Justin Erenkrantz wrote:
> --On October 21, 2005 4:29:36 PM +0200 Ruediger Pluem
>> - add my (of course non binding) vote on this backport?
>
>
> FWIW, your vote *is* binding and counts towards quorum. -- justin
Sorry, but I am confused. I thought only PMC members have binding votes.
Or is my vote binding because I proposed the backport?
Regards
Rüdiger
Re: [PATCH] PR37145: data loss with httpd-2.0.55 reverse proxy
method=post
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
--On October 21, 2005 4:29:36 PM +0200 Ruediger Pluem <rp...@apache.org>
wrote:
> like to propose this patch for backport. As I am only committer am I
> allowed
>
> - to add it to the 2.0.x STATUS file
> - add my (of course non binding) vote on this backport?
FWIW, your vote *is* binding and counts towards quorum. -- justin
Re: [PATCH] PR37145: data loss with httpd-2.0.55 reverse proxy method=post
Posted by Ruediger Pluem <rp...@apache.org>.
On 10/21/2005 04:06 PM, Joe Orton wrote:
[..cut..]
> I agree that's the correct analysis, your patch to fix the proxy to use
> ap_save_brigade looks good to me.
Thanks for feedback. I will commit later to give otherBill a chance for
feedback.
One technical question: As this bug was reported against 2.0.55 I would like
to propose this patch for backport. As I am only committer am I allowed
- to add it to the 2.0.x STATUS file
- add my (of course non binding) vote on this backport?
Regards
Rüdiger
Re: [PATCH] PR37145: data loss with httpd-2.0.55 reverse proxy method=post
Posted by Joe Orton <jo...@redhat.com>.
On Wed, Oct 19, 2005 at 11:44:26PM +0200, Ruediger Pluem wrote:
> Sorry, maybe I am only confused, but I think I disagree with you on that.
> The proxy code is reading the input filter chain in a loop and does repeated
> calls to ap_get_brigade without doing any more things with these brigades
> it gets from ap_get_brigade, but storing them for later processing. This
> looks to me like a typical setaside situation.
I agree that's the correct analysis, your patch to fix the proxy to use
ap_save_brigade looks good to me.
joe
Re: [PATCH] PR37145: data loss with httpd-2.0.55 reverse proxy method=post
Posted by Ruediger Pluem <rp...@apache.org>.
On 10/19/2005 10:44 PM, William A. Rowe, Jr. wrote:
[..cut..]
>
> The problem is -not- in creating the transient buckets (if they are
> sent, that's
> goodness). The problem is in transforming them to persistant buckets
> before the
> core, ssl, or other filters who have set-aside operations decide to
> return. So
> there should have been no trouble creating the transient bucket below, the
> trouble came in when the core filter didn't send the data, and also
> didn't set
> it aside :-(
Sorry, maybe I am only confused, but I think I disagree with you on that.
The proxy code is reading the input filter chain in a loop and does repeated
calls to ap_get_brigade without doing any more things with these brigades
it gets from ap_get_brigade, but storing them for later processing. This
looks to me like a typical setaside situation.
The data was not send because ap_proxy_http_request kept it for itself
not sending it anyware.
Regards
Rüdiger
[..cut..]
Re: [PATCH] PR37145: data loss with httpd-2.0.55 reverse proxy method=post
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Ruediger Pluem wrote:
>
> On 10/19/2005 08:25 PM, William A. Rowe, Jr. wrote:
>
> [..cut..]
>
>
>>Ruediger - I'm questioning if it's the BRIGADE_CONCAT, or actually if we
>>are
>>failing to respect transient buckets in the core / ssl filters. If we are
>>failing to pay attention to transient buckets (or not allocating the
>>buckets
>>as transient when, in fact, they are) then this might be the root bug.
>
>
> Agreed. The question is what we can expect from the core / ssl filters (Is
> there any kind of contract defined?). This defines if it is a bug in
> core / ssl or in the proxy code.
> So I guess the solution is either to handle transient buckets carefully later by setting
> them aside in this situation, as the current patches do or to adjust the
> core / ssl filters in a way that they do not deliver transient buckets any longer.
> The attached quick and dirty patch following this way seems to solve the problem too,
> but
>
> - I am not sure about its sideeffects.
> - There are further places in the SSL code where transient buckets get used. They
> would need to checked as well.
The problem is -not- in creating the transient buckets (if they are sent, that's
goodness). The problem is in transforming them to persistant buckets before the
core, ssl, or other filters who have set-aside operations decide to return. So
there should have been no trouble creating the transient bucket below, the
trouble came in when the core filter didn't send the data, and also didn't set
it aside :-(
Bill
> BTW: The problem does not happen without SSL because in this case we get heap buckets
> (checked with gdb, but not in the code).
>
> 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);
> }
Re: [PATCH] PR37145: data loss with httpd-2.0.55 reverse proxy method=post
Posted by Ruediger Pluem <rp...@apache.org>.
On 10/19/2005 08:25 PM, William A. Rowe, Jr. wrote:
[..cut..]
>
> Ruediger - I'm questioning if it's the BRIGADE_CONCAT, or actually if we
> are
> failing to respect transient buckets in the core / ssl filters. If we are
> failing to pay attention to transient buckets (or not allocating the
> buckets
> as transient when, in fact, they are) then this might be the root bug.
Agreed. The question is what we can expect from the core / ssl filters (Is
there any kind of contract defined?). This defines if it is a bug in
core / ssl or in the proxy code.
So I guess the solution is either to handle transient buckets carefully later by setting
them aside in this situation, as the current patches do or to adjust the
core / ssl filters in a way that they do not deliver transient buckets any longer.
The attached quick and dirty patch following this way seems to solve the problem too,
but
- I am not sure about its sideeffects.
- There are further places in the SSL code where transient buckets get used. They
would need to checked as well.
BTW: The problem does not happen without SSL because in this case we get heap buckets
(checked with gdb, but not in the code).
>
> Researching as well.
>
Regards
Rüdiger
Re: [PATCH] PR37145: data loss with httpd-2.0.55 reverse proxy method=post
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Ruediger Pluem wrote:
> 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 :-).
Ruediger - I'm questioning if it's the BRIGADE_CONCAT, or actually if we are
failing to respect transient buckets in the core / ssl filters. If we are
failing to pay attention to transient buckets (or not allocating the buckets
as transient when, in fact, they are) then this might be the root bug.
Researching as well.
Bill
Re: [PATCH] PR37145: data loss with httpd-2.0.55 reverse proxy method=post
Posted by Ruediger Pluem <rp...@apache.org>.
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
>>>
Re: [PATCH] PR37145: data loss with httpd-2.0.55 reverse proxy method=post
Posted by Ruediger Pluem <rp...@apache.org>.
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
>>
Re: [PATCH] PR37145
Posted by Ruediger Pluem <rp...@apache.org>.
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
>