Re: svn commit: r1782986 - /httpd/httpd/trunk/server/util_filter.c

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1782986 - /httpd/httpd/trunk/server/util_filter.c

Christophe JAILLET
Le 14/02/2017 à 17:43, [hidden email] a écrit :

> Author: ylavic
> Date: Tue Feb 14 16:43:25 2017
> New Revision: 1782986
>
> URL:http://svn.apache.org/viewvc?rev=1782986&view=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&r1=1782985&r2=1782986&view=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

Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1782986 - /httpd/httpd/trunk/server/util_filter.c

Ruediger Pluem


On 7/5/20 9:29 AM, Christophe JAILLET wrote:

> Le 14/02/2017 à 17:43, [hidden email] a écrit :
>> Author: ylavic
>> Date: Tue Feb 14 16:43:25 2017
>> New Revision: 1782986
>>
>> URL:http://svn.apache.org/viewvc?rev=1782986&view=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&r1=1782985&r2=1782986&view=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

Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1782986 - /httpd/httpd/trunk/server/util_filter.c

Yann Ylavic
On Mon, Jul 6, 2020 at 8:44 AM Ruediger Pluem <[hidden email]> wrote:

>
> On 7/5/20 9:29 AM, Christophe JAILLET wrote:
> >
> > Le 14/02/2017 à 17:43, [hidden email] 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 &apr_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.
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1782986 - /httpd/httpd/trunk/server/util_filter.c

Yann Ylavic
On Mon, Jul 6, 2020 at 11:56 AM Yann Ylavic <[hidden email]> 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 &apr_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.
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1782986 - /httpd/httpd/trunk/server/util_filter.c

Christophe JAILLET
In reply to this post by Yann Ylavic
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.