httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Yann Ylavic <ylavic....@gmail.com>
Subject Re: [Patch] Async write completion for the full connection filter stack
Date Sun, 14 Sep 2014 20:02:23 GMT
On Sat, Sep 13, 2014 at 7:03 PM, Graham Leggett <minfrin@sharp.fm> wrote:
> On 12 Sep 2014, at 4:57 PM, Yann Ylavic <ylavic.dev@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.

Mime
View raw message