From: Yann Ylavic Date: Tue, 31 Mar 2020 16:22:53 +0000 (+0000) Subject: core: handle morphing buckets setaside/reinstate and kill request core filter. X-Git-Tag: 2.5.0-alpha2-ci-test-only~1542 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=b3110d36afaceab45e083a68b1a8493b8244af2a;p=thirdparty%2Fapache%2Fhttpd.git core: handle morphing buckets setaside/reinstate and kill request core filter. The purpose of ap_request_core_filter() is not clear, it seems to prevent potential morphing buckets to go through AP_FTYPE_CONNECTION filters which would fail to set them aside (ENOTIMPL), and read them (unbounded) in memory. This patch allows ap_filter_setaside_brigade() to set morphing buckets aside by simply moving them, assuming they have the correct lifetime (either until some further EOR, or the connection lifetime, or whatever). IOW, the module is responsible for sending morphing buckets whose lifetime needs not be changed by the connection filters. Now since morphing buckets consume no memory until (apr_bucket_)read, like FILE buckets, we don't account for them in flush_max_threshold either. This changes ap_filter_reinstate_brigade() to only account for in-memory and EOR buckets to flush_upto. Also, since the EOR bucket is sent only to c->output_filters once the request is processed, when all the filters < AP_FTYPE_CONNECTION have done their job and stopped retaining data (after the EOS bucket, if ever), we prevent misuse of ap_filter_{setaside,reinstate}_brigade() outside connection filters by returning ENOTIMPL. This is not the right API for request filters as of now. Finally, ap_request_core_filter() and co can be removed. git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1875947 13f79535-47bb-0310-9956-ffa450edef68 --- diff --git a/include/http_request.h b/include/http_request.h index 5ca04b091d4..4259e8a6524 100644 --- a/include/http_request.h +++ b/include/http_request.h @@ -149,18 +149,6 @@ AP_DECLARE(int) ap_run_sub_req(request_rec *r); */ AP_DECLARE(void) ap_destroy_sub_req(request_rec *r); -/** - * An output filter to ensure that we avoid passing morphing buckets to - * connection filters and in so doing defeat async write completion when - * they are set aside. This should be inserted at the end of a request - * filter stack. - * @param f The current filter - * @param bb The brigade to filter - * @return status code - */ -AP_CORE_DECLARE_NONSTD(apr_status_t) ap_request_core_filter(ap_filter_t *f, - apr_bucket_brigade *bb); - /* * Then there's the case that you want some other request to be served * as the top-level request INSTEAD of what the client requested directly. @@ -600,6 +588,15 @@ AP_DECLARE(int) ap_if_walk(request_rec *r); /** End Of REQUEST (EOR) bucket */ AP_DECLARE_DATA extern const apr_bucket_type_t ap_bucket_type_eor; +/** + * Determine if a bucket is morphing, that is which changes its + * type on read (usually to "heap" allocated data), while moving + * itself at the next position to remain plugged until exhausted. + * @param e The bucket to inspect + * @return true or false + */ +#define AP_BUCKET_IS_MORPHING(e) ((e)->length == (apr_size_t)-1) + /** * Determine if a bucket is an End Of REQUEST (EOR) bucket * @param e The bucket to inspect diff --git a/modules/http/http_core.c b/modules/http/http_core.c index 746cf704a7b..3f87f80f954 100644 --- a/modules/http/http_core.c +++ b/modules/http/http_core.c @@ -268,8 +268,6 @@ static int http_create_request(request_rec *r) NULL, r, r->connection); ap_add_output_filter_handle(ap_http_outerror_filter_handle, NULL, r, r->connection); - ap_add_output_filter_handle(ap_request_core_filter_handle, - NULL, r, r->connection); } return OK; diff --git a/modules/http/http_request.c b/modules/http/http_request.c index 15c1fd51243..a5fdaf44f9b 100644 --- a/modules/http/http_request.c +++ b/modules/http/http_request.c @@ -350,7 +350,6 @@ AP_DECLARE(void) ap_process_request_after_handler(request_rec *r) apr_bucket_brigade *bb; apr_bucket *b; conn_rec *c = r->connection; - ap_filter_t *f; bb = ap_acquire_brigade(c); @@ -371,15 +370,9 @@ AP_DECLARE(void) ap_process_request_after_handler(request_rec *r) /* All the request filters should have bailed out on EOS, and in any * case they shouldn't have to handle this EOR which will destroy the - * request underneath them. So go straight to the core request filter - * which (if any) will take care of the setaside buckets. + * request underneath them. So go straight to the connection filters. */ - for (f = r->output_filters; f; f = f->next) { - if (f->frec == ap_request_core_filter_handle) { - break; - } - } - ap_pass_brigade(f ? f : c->output_filters, bb); + ap_pass_brigade(c->output_filters, bb); /* The EOR bucket has either been handled by an output filter (eg. * deleted or moved to a buffered_bb => no more in bb), or an error diff --git a/server/core.c b/server/core.c index 0fc4a8ef973..3a00761b76d 100644 --- a/server/core.c +++ b/server/core.c @@ -122,7 +122,6 @@ AP_IMPLEMENT_HOOK_RUN_FIRST(apr_status_t, insert_network_bucket, /* Handles for core filters */ AP_DECLARE_DATA ap_filter_rec_t *ap_subreq_core_filter_handle; -AP_DECLARE_DATA ap_filter_rec_t *ap_request_core_filter_handle; AP_DECLARE_DATA ap_filter_rec_t *ap_core_output_filter_handle; AP_DECLARE_DATA ap_filter_rec_t *ap_content_length_filter_handle; AP_DECLARE_DATA ap_filter_rec_t *ap_core_input_filter_handle; @@ -5916,9 +5915,6 @@ static void register_hooks(apr_pool_t *p) ap_core_output_filter_handle = ap_register_output_filter("CORE", ap_core_output_filter, NULL, AP_FTYPE_NETWORK); - ap_request_core_filter_handle = - ap_register_output_filter("REQ_CORE", ap_request_core_filter, - NULL, AP_FTYPE_CONNECTION - 1); ap_subreq_core_filter_handle = ap_register_output_filter("SUBREQ_CORE", ap_sub_req_output_filter, NULL, AP_FTYPE_CONTENT_SET); diff --git a/server/core_filters.c b/server/core_filters.c index bb02b207fab..0c08303c7a3 100644 --- a/server/core_filters.c +++ b/server/core_filters.c @@ -543,6 +543,12 @@ static apr_status_t send_brigade_nonblocking(apr_socket_t *s, rv = apr_bucket_read(bucket, &data, &length, APR_BLOCK_READ); } + if (APR_STATUS_IS_EOF(rv)) { + /* Morphing bucket exhausted, next. */ + apr_bucket_delete(bucket); + rv = APR_SUCCESS; + continue; + } if (rv != APR_SUCCESS) { goto cleanup; } diff --git a/server/request.c b/server/request.c index a448fa7af5f..d06bab5d311 100644 --- a/server/request.c +++ b/server/request.c @@ -2058,124 +2058,6 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_sub_req_output_filter(ap_filter_t *f, return APR_SUCCESS; } -AP_CORE_DECLARE_NONSTD(apr_status_t) ap_request_core_filter(ap_filter_t *f, - apr_bucket_brigade *bb) -{ - apr_status_t status = APR_SUCCESS; - apr_read_type_e block = APR_NONBLOCK_READ; - conn_rec *c = f->r->connection; - apr_bucket *flush_upto = NULL; - apr_bucket_brigade *tmp_bb; - apr_size_t tmp_bb_len = 0; - core_server_config *conf; - int seen_eor = 0; - - /* - * Handle the AsyncFilter directive. We limit the filters that are - * eligible for asynchronous handling here. - */ - if (f->frec->ftype < f->c->async_filter) { - ap_remove_output_filter(f); - return ap_pass_brigade(f->next, bb); - } - - conf = ap_get_core_module_config(f->r->server->module_config); - - /* Reinstate any buffered content */ - ap_filter_reinstate_brigade(f, bb, &flush_upto); - - /* After EOR is passed downstream, anything pooled on the request may - * be destroyed (including bb!), but not tmp_bb which is acquired from - * c->pool (and released after the below loop). - */ - tmp_bb = ap_acquire_brigade(f->c); - - /* Don't touch *bb after seen_eor */ - while (status == APR_SUCCESS && !seen_eor && !APR_BRIGADE_EMPTY(bb)) { - apr_bucket *bucket = APR_BRIGADE_FIRST(bb); - int do_pass = 0; - - if (AP_BUCKET_IS_EOR(bucket)) { - /* pass out everything and never come back again, - * r is destroyed with this bucket! - */ - APR_BRIGADE_CONCAT(tmp_bb, bb); - ap_remove_output_filter(f); - seen_eor = 1; - } - else { - /* if the core has set aside data, back off and try later */ - if (!flush_upto) { - if (ap_filter_should_yield(f->next)) { - break; - } - } - else if (flush_upto == bucket) { - flush_upto = NULL; - } - - /* have we found a morphing bucket? if so, force it to morph into - * something safe to pass down to the connection filters without - * needing to be set aside. - */ - if (bucket->length == (apr_size_t)-1) { - const char *data; - apr_size_t size; - - status = apr_bucket_read(bucket, &data, &size, block); - if (status != APR_SUCCESS) { - if (!APR_STATUS_IS_EAGAIN(status) - || block != APR_NONBLOCK_READ) { - break; - } - /* Flush everything so far and retry in blocking mode */ - bucket = apr_bucket_flush_create(c->bucket_alloc); - block = APR_BLOCK_READ; - } - else { - tmp_bb_len += size; - block = APR_NONBLOCK_READ; - } - } - else { - tmp_bb_len += bucket->length; - } - - /* move the bucket to tmp_bb and check whether it exhausts bb or - * brings tmp_bb above the limit; in both cases it's time to pass - * everything down the chain. - */ - APR_BUCKET_REMOVE(bucket); - APR_BRIGADE_INSERT_TAIL(tmp_bb, bucket); - if (APR_BRIGADE_EMPTY(bb) - || APR_BUCKET_IS_FLUSH(bucket) - || tmp_bb_len >= conf->flush_max_threshold) { - do_pass = 1; - } - } - - if (do_pass || seen_eor) { - status = ap_pass_brigade(f->next, tmp_bb); - apr_brigade_cleanup(tmp_bb); - tmp_bb_len = 0; - } - } - - /* Don't touch *bb after seen_eor */ - if (!seen_eor) { - apr_status_t rv; - APR_BRIGADE_PREPEND(bb, tmp_bb); - rv = ap_filter_setaside_brigade(f, bb); - if (status == APR_SUCCESS) { - status = rv; - } - } - - ap_release_brigade(f->c, tmp_bb); - - return status; -} - extern APR_OPTIONAL_FN_TYPE(authz_some_auth_required) *ap__authz_ap_some_auth_required; AP_DECLARE(int) ap_some_auth_required(request_rec *r) diff --git a/server/util_filter.c b/server/util_filter.c index 59e2e99d026..8f34a8771c6 100644 --- a/server/util_filter.c +++ b/server/util_filter.c @@ -934,9 +934,20 @@ AP_DECLARE(int) ap_filter_prepare_brigade(ap_filter_t *f) return OK; } +static apr_status_t save_aside_brigade(struct ap_filter_private *fp, + apr_bucket_brigade *bb) +{ + if (!fp->deferred_pool) { + apr_pool_create(&fp->deferred_pool, fp->f->c->pool); + apr_pool_tag(fp->deferred_pool, "deferred_pool"); + } + return ap_save_brigade(fp->f, &fp->bb, &bb, fp->deferred_pool); +} + AP_DECLARE(apr_status_t) ap_filter_setaside_brigade(ap_filter_t *f, apr_bucket_brigade *bb) { + apr_status_t rv = APR_SUCCESS; struct ap_filter_private *fp = f->priv; ap_log_cerror(APLOG_MARK, APLOG_TRACE6, 0, f->c, @@ -945,34 +956,83 @@ AP_DECLARE(apr_status_t) ap_filter_setaside_brigade(ap_filter_t *f, (!fp->bb || APR_BRIGADE_EMPTY(fp->bb)) ? "empty" : "full", f->frec->name); + /* This API is not suitable for request filters */ + if (f->frec->ftype < AP_FTYPE_CONNECTION) { + return APR_ENOTIMPL; + } + if (!APR_BRIGADE_EMPTY(bb)) { + apr_bucket_brigade *tmp_bb = NULL; + int batched_buckets = 0; + apr_bucket *e, *next; + /* - * Set aside the brigade bb within fp->bb. + * Set aside the brigade bb to fp->bb. */ ap_filter_prepare_brigade(f); - /* decide what pool we setaside to, request pool or deferred pool? */ - if (f->r) { - apr_bucket *e; - for (e = APR_BRIGADE_FIRST(bb); e != APR_BRIGADE_SENTINEL(bb); e = - APR_BUCKET_NEXT(e)) { - if (APR_BUCKET_IS_TRANSIENT(e)) { - int rv = apr_bucket_setaside(e, f->r->pool); + for (e = APR_BRIGADE_FIRST(bb); + e != APR_BRIGADE_SENTINEL(bb); + e = next) { + next = APR_BUCKET_NEXT(e); + + /* Morphing buckets are moved, so assumed to have next EOR's + * lifetime or at least the lifetime of the connection. + */ + if (AP_BUCKET_IS_MORPHING(e)) { + /* Save buckets batched below? */ + if (batched_buckets) { + batched_buckets = 0; + if (!tmp_bb) { + tmp_bb = ap_acquire_brigade(f->c); + } + apr_brigade_split_ex(bb, e, tmp_bb); + rv = save_aside_brigade(fp, bb); + APR_BRIGADE_CONCAT(bb, tmp_bb); if (rv != APR_SUCCESS) { - return rv; + break; } + AP_DEBUG_ASSERT(APR_BRIGADE_FIRST(bb) == e); } + APR_BUCKET_REMOVE(e); + APR_BRIGADE_INSERT_TAIL(fp->bb, e); } - APR_BRIGADE_CONCAT(fp->bb, bb); - } - else { - if (!fp->deferred_pool) { - apr_pool_create(&fp->deferred_pool, f->c->pool); - apr_pool_tag(fp->deferred_pool, "deferred_pool"); + else { + /* Batch successive buckets to save. */ + batched_buckets = 1; } - return ap_save_brigade(f, &fp->bb, &bb, fp->deferred_pool); } - + if (tmp_bb) { + ap_release_brigade(f->c, tmp_bb); + } + if (batched_buckets) { + /* Save any remainder. */ + rv = save_aside_brigade(fp, bb); + } + if (!APR_BRIGADE_EMPTY(bb)) { + /* Anything left in bb is what we could not save (error), clean up. + * This destroys anything pipelined so far, including EOR(s), and + * swallows all data, so from now this filter should only be passed + * connection close data like TLS close_notify. + * + * XXX: Should we cleanup all previous c->output_filters' setaside + * brigades? + * + * XXX: For each EOR we potentially destroy here, there is a + * request handler/module which "thought" everything went well + * on the output filters side, and returned OK. Should we mark + * something in each EOR's request_rec (e.g. r->aborted) for + * the log_transaction hooks to know at least? + * Or alternatively (and possibly more robustly) have the + * ap_core_output_filter() set r->flushed when it sees an EOR + * up to which it sent everything (before destroying it)? + * Anyway we can't set c->aborted here, because close_notify + * for instance can/should still be sent out. + */ + AP_DEBUG_ASSERT(rv != APR_SUCCESS); + f->c->keepalive = AP_CONN_CLOSE; + apr_brigade_cleanup(bb); + } } else if (fp->deferred_pool) { /* @@ -983,7 +1043,8 @@ AP_DECLARE(apr_status_t) ap_filter_setaside_brigade(ap_filter_t *f, apr_brigade_cleanup(fp->bb); apr_pool_clear(fp->deferred_pool); } - return APR_SUCCESS; + + return rv; } void ap_filter_adopt_brigade(ap_filter_t *f, apr_bucket_brigade *bb) @@ -1007,8 +1068,8 @@ AP_DECLARE(apr_status_t) ap_filter_reinstate_brigade(ap_filter_t *f, apr_bucket **flush_upto) { apr_bucket *bucket, *next; - apr_size_t bytes_in_brigade, non_file_bytes_in_brigade; - int eor_buckets_in_brigade, morphing_bucket_in_brigade; + apr_size_t bytes_in_brigade, memory_bytes_in_brigade; + int eor_buckets_in_brigade, morphing_buckets_in_brigade; struct ap_filter_private *fp = f->priv; core_server_config *conf; @@ -1018,6 +1079,14 @@ AP_DECLARE(apr_status_t) ap_filter_reinstate_brigade(ap_filter_t *f, (APR_BRIGADE_EMPTY(bb) ? "empty" : "full"), f->frec->name); + /* This API is not suitable for request filters */ + if (f->frec->ftype < AP_FTYPE_CONNECTION) { + return APR_ENOTIMPL; + } + + /* Buckets in fp->bb are leftover from previous call to setaside, so + * they happen before anything added here in bb. + */ if (fp->bb) { APR_BRIGADE_PREPEND(bb, fp->bb); } @@ -1029,39 +1098,35 @@ AP_DECLARE(apr_status_t) ap_filter_reinstate_brigade(ap_filter_t *f, *flush_upto = NULL; /* - * Determine if and up to which bucket we need to do a blocking write: + * Determine if and up to which bucket the caller needs to do a blocking + * write: * - * a) The brigade contains a flush bucket: Do a blocking write - * of everything up that point. + * a) The brigade contains at least one flush bucket: do blocking writes + * of everything up to the last one. * - * b) The request is in CONN_STATE_HANDLER state, and the brigade - * contains at least flush_max_threshold bytes in non-file - * buckets: Do blocking writes until the amount of data in the - * buffer is less than flush_max_threshold. (The point of this - * rule is to provide flow control, in case a handler is - * streaming out lots of data faster than the data can be + * b) The brigade contains at least flush_max_threshold bytes in memory, + * that is non-file and non-morphing buckets: do blocking writes of + * everything up the last bucket above flush_max_threshold. + * (The point of this rule is to provide flow control, in case a + * handler is streaming out lots of data faster than the data can be * sent to the client.) * - * c) The request is in CONN_STATE_HANDLER state, and the brigade - * contains at least flush_max_pipelined EOR buckets: - * Do blocking writes until less than flush_max_pipelined EOR - * buckets are left. (The point of this rule is to prevent too many - * FDs being kept open by pipelined requests, possibly allowing a - * DoS). + * c) The brigade contains at least flush_max_pipelined EOR buckets: do + * blocking writes until the last EOR above flush_max_pipelined. + * (The point of this rule is to prevent too many FDs being kept open + * by pipelined requests, possibly allowing a DoS). * - * d) The request is being served by a connection filter and the - * brigade contains a morphing bucket: If there was no other - * reason to do a blocking write yet, try reading the bucket. If its - * contents fit into memory before flush_max_threshold is reached, - * everything is fine. Otherwise we need to do a blocking write the - * up to and including the morphing bucket, because ap_save_brigade() - * would read the whole bucket into memory later on. + * Note that morphing buckets use no memory until read, so they don't + * account for point b) above. Both ap_filter_reinstate_brigade() and + * ap_filter_setaside_brigade() assume that morphing buckets have an + * appropriate lifetime (until next EOR for instance), so they are simply + * setaside or reinstated by moving them from/to fp->bb to/from bb. */ bytes_in_brigade = 0; - non_file_bytes_in_brigade = 0; + memory_bytes_in_brigade = 0; eor_buckets_in_brigade = 0; - morphing_bucket_in_brigade = 0; + morphing_buckets_in_brigade = 0; conf = ap_get_core_module_config(f->c->base_server->module_config); @@ -1069,50 +1134,41 @@ AP_DECLARE(apr_status_t) ap_filter_reinstate_brigade(ap_filter_t *f, bucket = next) { next = APR_BUCKET_NEXT(bucket); - if (!APR_BUCKET_IS_METADATA(bucket)) { - if (bucket->length == (apr_size_t)-1) { - /* - * A setaside of morphing buckets would read everything into - * memory. Instead, we will flush everything up to and - * including this bucket. - */ - morphing_bucket_in_brigade = 1; - } - else { - bytes_in_brigade += bucket->length; - if (!APR_BUCKET_IS_FILE(bucket)) - non_file_bytes_in_brigade += bucket->length; - } - } - else if (AP_BUCKET_IS_EOR(bucket)) { + if (AP_BUCKET_IS_EOR(bucket)) { eor_buckets_in_brigade++; } + else if (AP_BUCKET_IS_MORPHING(bucket)) { + morphing_buckets_in_brigade++; + } + else if (bucket->length) { + bytes_in_brigade += bucket->length; + if (!APR_BUCKET_IS_FILE(bucket)) { + memory_bytes_in_brigade += bucket->length; + } + } if (APR_BUCKET_IS_FLUSH(bucket) - || non_file_bytes_in_brigade >= conf->flush_max_threshold - || (!f->r && morphing_bucket_in_brigade) - || eor_buckets_in_brigade > conf->flush_max_pipelined) { + || memory_bytes_in_brigade >= conf->flush_max_threshold + || eor_buckets_in_brigade >= conf->flush_max_pipelined) { /* this segment of the brigade MUST be sent before returning. */ if (APLOGctrace6(f->c)) { char *reason = APR_BUCKET_IS_FLUSH(bucket) ? "FLUSH bucket" : - (non_file_bytes_in_brigade >= conf->flush_max_threshold) ? - "max threshold" : - (!f->r && morphing_bucket_in_brigade) ? "morphing bucket" : - "max requests in pipeline"; + (memory_bytes_in_brigade >= conf->flush_max_threshold) ? + "max threshold" : "max requests in pipeline"; ap_log_cerror(APLOG_MARK, APLOG_TRACE6, 0, f->c, "will flush because of %s", reason); ap_log_cerror(APLOG_MARK, APLOG_TRACE8, 0, f->c, "seen in brigade%s: bytes: %" APR_SIZE_T_FMT - ", non-file bytes: %" APR_SIZE_T_FMT ", eor " + ", memory bytes: %" APR_SIZE_T_FMT ", eor " "buckets: %d, morphing buckets: %d", *flush_upto == NULL ? " so far" : " since last flush point", bytes_in_brigade, - non_file_bytes_in_brigade, + memory_bytes_in_brigade, eor_buckets_in_brigade, - morphing_bucket_in_brigade); + morphing_buckets_in_brigade); } /* * Defer the actual blocking write to avoid doing many writes. @@ -1120,18 +1176,19 @@ AP_DECLARE(apr_status_t) ap_filter_reinstate_brigade(ap_filter_t *f, *flush_upto = next; bytes_in_brigade = 0; - non_file_bytes_in_brigade = 0; + memory_bytes_in_brigade = 0; eor_buckets_in_brigade = 0; - morphing_bucket_in_brigade = 0; + morphing_buckets_in_brigade = 0; } } ap_log_cerror(APLOG_MARK, APLOG_TRACE8, 0, f->c, - "brigade contains: bytes: %" APR_SIZE_T_FMT + "brigade contains%s: bytes: %" APR_SIZE_T_FMT ", non-file bytes: %" APR_SIZE_T_FMT ", eor buckets: %d, morphing buckets: %d", - bytes_in_brigade, non_file_bytes_in_brigade, - eor_buckets_in_brigade, morphing_bucket_in_brigade); + *flush_upto == NULL ? "" : " since last flush point", + bytes_in_brigade, memory_bytes_in_brigade, + eor_buckets_in_brigade, morphing_buckets_in_brigade); return APR_SUCCESS; } @@ -1200,8 +1257,8 @@ AP_DECLARE_NONSTD(int) ap_filter_output_pending(conn_rec *c) } /* Flush outer most filters first for ap_filter_should_yield(f->next) - * to be relevant in the previous ones (e.g. ap_request_core_filter() - * won't pass its buckets if its next filters yield already). + * to be relevant in the previous ones (async filters won't pass their + * buckets if their next filters yield already). */ bb = ap_acquire_brigade(c); for (fp = APR_RING_LAST(x->pending_output_filters);