On Sat, Sep 13, 2014 at 7:03 PM, Graham Leggett <minf...@sharp.fm> wrote: > On 12 Sep 2014, at 4:57 PM, Yann Ylavic <ylavic....@gmail.com> wrote: > >> Makes sense, however ap_core_output_filter() and >> ssl_io_filter_output() *know* that their buffered_bb does not contain >> such bucket (exclusively filled by ap_filter_setaside_brigade()), in >> that the original code (still in svn) is more optimal since it does >> not walk through the same brigade multiple times (doing an immediate >> nonblocking write with what it has and returning). > > I suppose if we clearly document the behaviour it should be fine. We already > force the return of should_write if an empty brigade was passed in (as in, “no > more coming for now, just write it”).
If an empty brigade is given but buffered_bb (controlled and hence possibly mangled by the user) contains a flush bucket, we will end up writing up to this bucket in blocking mode and the remaining (if any) in nonblocking, while the original code does it all nonblocking (because it knows its buffered_bb is ok). > >> To avoid that and the buffered_bb lifetime issue discussed with >> Rüdiger, I'm thinking of an opaque type (a struct containing the >> buffered_bb and the deferred_pool) that could be passed to both >> ap_filter_setaside_brigade() and ap_filter_reinstate_brigade(). >> That way no user could modify the content of the brigade or play with >> the pool (opaque), so that ap_filter_setaside_brigade() could >> create/clear the deferred_pool as it wish, and >> ap_filter_reinstate_brigade() could be sure buffered_bb contains only >> suitable buckets. > > Are we not just trying to solve a dangling pointer to a brigade by replacing > it with a dangling pointer to apr_filter_aside_t? I don't think so, the only way to free an apr_filter_aside_t (outside util_filter.c) is to clear c->pool, which any sane filter will never do (no need to document, that's httpd's job). Either the apr_filter_aside_t* is NULL, or it's fields are solely controlled by ap_filter_setaside_brigade() / ap_filter_reinstate_brigade(). Do the right thing in these functions and everything is fine. Regards, Yann.
