Re: svn commit: r1782986 - /httpd/httpd/trunk/server/util_filter.c
Thx for both of you for the explanation.
Le 06/07/2020 à 11:56, Yann Ylavic a écrit :
[...]
Finally APR_BUCKET_IS_{EOS,}(e) on an EMPTY brigade is always false
with the current struct apr_bucket_brigade API.
That's also my understanding. That is what was puzzling me.
Just a bit fragile :)
Ok. Better safe than sorry.
CJ
Regards;
Yann.
Re: svn commit: r1782986 - /httpd/httpd/trunk/server/util_filter.c
On Mon, Jul 6, 2020 at 11:56 AM Yann Ylavic wrote:
> >
> > In the previous code the first condition in the if was always true, and I
> > am not sure what happened with the second condition in
> > case e was the sentinel.
>
> AIUI, dereferencing the SENTINEL is not accessing unreserved/freed
> memory, it's accessing the RING/BRIGADE head (here bb->list the
> placeholder for `struct {apr_bucket *next, *prev;}`) as if it were an
> apr_bucket (given that struct apr_bucket has its own head/placeholder,
> e->type is `sizeof(apr_bucket*)` bytes after bb->list)).
s/`sizeof(apr_bucket*)` bytes/just/
> That's `apr_bucket_alloc_t *bucket_alloc` in struct
> apr_bucket_brigade, so quite unlikely to be _bucket_type_eos.
> Finally APR_BUCKET_IS_{EOS,}(e) on an EMPTY brigade is always false
> with the current struct apr_bucket_brigade API. Just a bit fragile :)
>
>
> Regards;
> Yann.
Re: svn commit: r1782986 - /httpd/httpd/trunk/server/util_filter.c
On Mon, Jul 6, 2020 at 8:44 AM Ruediger Pluem wrote:
>
> On 7/5/20 9:29 AM, Christophe JAILLET wrote:
> >
> > Le 14/02/2017 à 17:43, yla...@apache.org a écrit :
> >> @@ -584,9 +584,9 @@ AP_DECLARE(apr_status_t) ap_pass_brigade
> >>apr_bucket_brigade *bb)
> >> {
> >> if (next) {
> >> -apr_bucket *e;
> >> +apr_bucket *e = APR_BRIGADE_LAST(bb);
> >> -if ((e = APR_BRIGADE_LAST(bb)) && APR_BUCKET_IS_EOS(e) &&
> >> next->r) {
> >> +if (e != APR_BRIGADE_SENTINEL(bb) && APR_BUCKET_IS_EOS(e) &&
> >> next->r) {
> >> /* This is only safe because HTTP_HEADER filter is always in
> >>* the filter stack. This ensures that there is ALWAYS a
> >>* request-based filter that we can attach this to. If the
> >
> > Hi,
> >
> > I don't really understand the change above.
> >
> > The commit description is clear. 'ap_pass_brigade' should deal better with
> > empty brigades.
> >
> > However, if the last bucket is an EOS bucket, how could it be a sentinel?
> > My understanding is that this change is a no-op, but I may have missed
> > something.
> >
> > What is the rational for it?
>
> If the brigade is empty then APR_BRIGADE_LAST(bb) points to the sentinel.
> Hence The first condition of the if is not met and we
> jump over the block. If the brigade is not empty we check if the last bucket
> which is now not the sentinel is the eos bucket.
Yeah, the commit replaced e != NULL (always true) with e != SENTINEL
(the relevant test to avoid dereferencing the sentinel in
APR_BUCKET_IS_EOS).
>
> In the previous code the first condition in the if was always true, and I am
> not sure what happened with the second condition in
> case e was the sentinel.
AIUI, dereferencing the SENTINEL is not accessing unreserved/freed
memory, it's accessing the RING/BRIGADE head (here bb->list the
placeholder for `struct {apr_bucket *next, *prev;}`) as if it were an
apr_bucket (given that struct apr_bucket has its own head/placeholder,
e->type is `sizeof(apr_bucket*)` bytes after bb->list)).
That's `apr_bucket_alloc_t *bucket_alloc` in struct
apr_bucket_brigade, so quite unlikely to be _bucket_type_eos.
Finally APR_BUCKET_IS_{EOS,}(e) on an EMPTY brigade is always false
with the current struct apr_bucket_brigade API. Just a bit fragile :)
Regards;
Yann.
Re: svn commit: r1782986 - /httpd/httpd/trunk/server/util_filter.c
On 7/5/20 9:29 AM, Christophe JAILLET wrote:
> Le 14/02/2017 à 17:43, yla...@apache.org a écrit :
>> Author: ylavic
>> Date: Tue Feb 14 16:43:25 2017
>> New Revision: 1782986
>>
>> URL:http://svn.apache.org/viewvc?rev=1782986=rev
>> Log:
>> util_filter: better ap_pass_brigade() vs empty brigades.
>>
>> Modified:
>> httpd/httpd/trunk/server/util_filter.c
>>
>> Modified: httpd/httpd/trunk/server/util_filter.c
>> URL:http://svn.apache.org/viewvc/httpd/httpd/trunk/server/util_filter.c?rev=1782986=1782985=1782986=diff
>> ==
>> --- httpd/httpd/trunk/server/util_filter.c (original)
>> +++ httpd/httpd/trunk/server/util_filter.c Tue Feb 14 16:43:25 2017
>> @@ -584,9 +584,9 @@ AP_DECLARE(apr_status_t) ap_pass_brigade
>> apr_bucket_brigade *bb)
>> {
>> if (next) {
>> - apr_bucket *e;
>> + apr_bucket *e = APR_BRIGADE_LAST(bb);
>> - if ((e = APR_BRIGADE_LAST(bb)) && APR_BUCKET_IS_EOS(e) &&
>> next->r) {
>> + if (e != APR_BRIGADE_SENTINEL(bb) && APR_BUCKET_IS_EOS(e) &&
>> next->r) {
>> /* This is only safe because HTTP_HEADER filter is always in
>> * the filter stack. This ensures that there is ALWAYS a
>> * request-based filter that we can attach this to. If the
>
> Hi,
>
> I don't really understand the change above.
>
> The commit description is clear. 'ap_pass_brigade' should deal better with
> empty brigades.
>
> However, if the last bucket is an EOS bucket, how could it be a sentinel?
> My understanding is that this change is a no-op, but I may have missed
> something.
>
> What is the rational for it?
If the brigade is empty then APR_BRIGADE_LAST(bb) points to the sentinel. Hence
The first condition of the if is not met and we
jump over the block. If the brigade is not empty we check if the last bucket
which is now not the sentinel is the eos bucket.
In the previous code the first condition in the if was always true, and I am
not sure what happened with the second condition in
case e was the sentinel.
Regards
Rüdiger
Re: svn commit: r1782986 - /httpd/httpd/trunk/server/util_filter.c
Le 14/02/2017 à 17:43, yla...@apache.org a écrit :
Author: ylavic
Date: Tue Feb 14 16:43:25 2017
New Revision: 1782986
URL:http://svn.apache.org/viewvc?rev=1782986=rev
Log:
util_filter: better ap_pass_brigade() vs empty brigades.
Modified:
httpd/httpd/trunk/server/util_filter.c
Modified: httpd/httpd/trunk/server/util_filter.c
URL:http://svn.apache.org/viewvc/httpd/httpd/trunk/server/util_filter.c?rev=1782986=1782985=1782986=diff
==
--- httpd/httpd/trunk/server/util_filter.c (original)
+++ httpd/httpd/trunk/server/util_filter.c Tue Feb 14 16:43:25 2017
@@ -584,9 +584,9 @@ AP_DECLARE(apr_status_t) ap_pass_brigade
apr_bucket_brigade *bb)
{
if (next) {
-apr_bucket *e;
+apr_bucket *e = APR_BRIGADE_LAST(bb);
-if ((e = APR_BRIGADE_LAST(bb)) && APR_BUCKET_IS_EOS(e) && next->r) {
+if (e != APR_BRIGADE_SENTINEL(bb) && APR_BUCKET_IS_EOS(e) && next->r) {
/* This is only safe because HTTP_HEADER filter is always in
* the filter stack. This ensures that there is ALWAYS a
* request-based filter that we can attach this to. If the
Hi,
I don't really understand the change above.
The commit description is clear. 'ap_pass_brigade' should deal better
with empty brigades.
However, if the last bucket is an EOS bucket, how could it be a sentinel?
My understanding is that this change is a no-op, but I may have missed
something.
What is the rational for it?
CJ
