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.