On Mon, Jul 6, 2020 at 8:44 AM Ruediger Pluem <rpl...@apache.org> 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 &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.
