From 19aa2d83b379719420f3a178413325156d7a62f3 Mon Sep 17 00:00:00 2001 From: Yann Ylavic Date: Mon, 7 Mar 2022 14:46:08 +0000 Subject: [PATCH] core: Simpler connection close logic if discarding the request body fails. If ap_discard_request_body() sets AP_CONN_CLOSE by itself it simplifies and allows to consolidate end_output_stream() and error_output_stream(). Merge r1898683 from trunk. Submitted by: ylavic, rpluem Reviewed by: ylavic, rpluem, covener git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.4.x@1898692 13f79535-47bb-0310-9956-ffa450edef68 --- changes-entries/discard_body.diff | 2 + modules/http/http_filters.c | 69 ++++++++++++++++--------------- server/protocol.c | 14 +++++-- 3 files changed, 48 insertions(+), 37 deletions(-) create mode 100644 changes-entries/discard_body.diff diff --git a/changes-entries/discard_body.diff b/changes-entries/discard_body.diff new file mode 100644 index 0000000000..6b467ac5ee --- /dev/null +++ b/changes-entries/discard_body.diff @@ -0,0 +1,2 @@ + *) core: Simpler connection close logic if discarding the request body fails. + [Yann Ylavic, Ruediger Pluem] \ No newline at end of file diff --git a/modules/http/http_filters.c b/modules/http/http_filters.c index d9b3621215..43e8c6dd5d 100644 --- a/modules/http/http_filters.c +++ b/modules/http/http_filters.c @@ -1598,9 +1598,9 @@ AP_DECLARE(int) ap_map_http_request_error(apr_status_t rv, int status) */ AP_DECLARE(int) ap_discard_request_body(request_rec *r) { + int rc = OK; + conn_rec *c = r->connection; apr_bucket_brigade *bb; - int seen_eos; - apr_status_t rv; /* Sometimes we'll get in a state where the input handling has * detected an error where we want to drop the connection, so if @@ -1609,54 +1609,57 @@ AP_DECLARE(int) ap_discard_request_body(request_rec *r) * * This function is also a no-op on a subrequest. */ - if (r->main || r->connection->keepalive == AP_CONN_CLOSE || - ap_status_drops_connection(r->status)) { + if (r->main || c->keepalive == AP_CONN_CLOSE) { + return OK; + } + if (ap_status_drops_connection(r->status)) { + c->keepalive = AP_CONN_CLOSE; return OK; } bb = apr_brigade_create(r->pool, r->connection->bucket_alloc); - seen_eos = 0; - do { - apr_bucket *bucket; + for (;;) { + apr_status_t rv; rv = ap_get_brigade(r->input_filters, bb, AP_MODE_READBYTES, APR_BLOCK_READ, HUGE_STRING_LEN); - if (rv != APR_SUCCESS) { - apr_brigade_destroy(bb); - return ap_map_http_request_error(rv, HTTP_BAD_REQUEST); + rc = ap_map_http_request_error(rv, HTTP_BAD_REQUEST); + goto cleanup; } - for (bucket = APR_BRIGADE_FIRST(bb); - bucket != APR_BRIGADE_SENTINEL(bb); - bucket = APR_BUCKET_NEXT(bucket)) - { - const char *data; - apr_size_t len; + while (!APR_BRIGADE_EMPTY(bb)) { + apr_bucket *b = APR_BRIGADE_FIRST(bb); - if (APR_BUCKET_IS_EOS(bucket)) { - seen_eos = 1; - break; - } - - /* These are metadata buckets. */ - if (bucket->length == 0) { - continue; + if (APR_BUCKET_IS_EOS(b)) { + goto cleanup; } - /* We MUST read because in case we have an unknown-length - * bucket or one that morphs, we want to exhaust it. + /* There is no need to read empty or metadata buckets or + * buckets of known length, but we MUST read buckets of + * unknown length in order to exhaust them. */ - rv = apr_bucket_read(bucket, &data, &len, APR_BLOCK_READ); - if (rv != APR_SUCCESS) { - apr_brigade_destroy(bb); - return HTTP_BAD_REQUEST; + if (b->length == (apr_size_t)-1) { + apr_size_t len; + const char *data; + + rv = apr_bucket_read(b, &data, &len, APR_BLOCK_READ); + if (rv != APR_SUCCESS) { + rc = HTTP_BAD_REQUEST; + goto cleanup; + } } + + apr_bucket_delete(b); } - apr_brigade_cleanup(bb); - } while (!seen_eos); + } - return OK; +cleanup: + apr_brigade_cleanup(bb); + if (rc != OK) { + c->keepalive = AP_CONN_CLOSE; + } + return rc; } /* Here we deal with getting the request message body from the client. diff --git a/server/protocol.c b/server/protocol.c index 2214f72b5a..298f61e1fb 100644 --- a/server/protocol.c +++ b/server/protocol.c @@ -1687,23 +1687,29 @@ AP_DECLARE(void) ap_set_sub_req_protocol(request_rec *rnew, rnew->main = (request_rec *) r; } -static void end_output_stream(request_rec *r) +static void end_output_stream(request_rec *r, int status) { conn_rec *c = r->connection; apr_bucket_brigade *bb; apr_bucket *b; bb = apr_brigade_create(r->pool, c->bucket_alloc); + if (status != OK) { + b = ap_bucket_error_create(status, NULL, r->pool, c->bucket_alloc); + APR_BRIGADE_INSERT_TAIL(bb, b); + } b = apr_bucket_eos_create(c->bucket_alloc); APR_BRIGADE_INSERT_TAIL(bb, b); + ap_pass_brigade(r->output_filters, bb); + apr_brigade_cleanup(bb); } AP_DECLARE(void) ap_finalize_sub_req_protocol(request_rec *sub) { /* tell the filter chain there is no more content coming */ if (!sub->eos_sent) { - end_output_stream(sub); + end_output_stream(sub, OK); } } @@ -1714,11 +1720,11 @@ AP_DECLARE(void) ap_finalize_sub_req_protocol(request_rec *sub) */ AP_DECLARE(void) ap_finalize_request_protocol(request_rec *r) { - (void) ap_discard_request_body(r); + int status = ap_discard_request_body(r); /* tell the filter chain there is no more content coming */ if (!r->eos_sent) { - end_output_stream(r); + end_output_stream(r, status); } } -- 2.30.2