Le 07/06/2021 à 17:34, Salvatore Bonaccorso a écrit : > Source: apache2 > Version: 2.4.47-1 > Severity: grave > Tags: security upstream > Justification: user security hole > X-Debbugs-Cc: car...@debian.org, Debian Security Team > <t...@security.debian.org> > > Hi, > > The following vulnerability was published for apache2. > > CVE-2021-31618[0]: > | httpd: NULL pointer dereference on specially crafted HTTP/2 request > > If you fix the vulnerability please also make sure to include the > CVE (Common Vulnerabilities & Exposures) id in your changelog entry. > > For further information see: > > [0] https://security-tracker.debian.org/tracker/CVE-2021-31618 > https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2021-31618 > [1] > https://github.com/apache/httpd/commit/a4fba223668c554e06bc78d6e3a88f33d4238ae4 > [2] https://httpd.apache.org/security/vulnerabilities_24.html#CVE-2021-31618 > > Please adjust the affected versions in the BTS as needed. > > Regards, > Salvatore
Hi all, I can't import the whole patch for Bullseye since it is written for 2.4.47. I think the best solution is to import the whole http2 module in Bullseye. This gives the attached patch Cheers, Yadd
Description: import the whole HTTP/2 module from 2.4.47 to fix CVE-2021-31618 Author: Xavier Guimard <y...@debian.org> Origin: upstream Bug: https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2021-31618 Bug-Debian: https://bugs.debian.org/989562 Forwarded: not-needed Reviewed-By: Yadd <y...@debian.org> Last-Update: 2021-06-08 --- a/modules/http2/h2.h +++ b/modules/http2/h2.h @@ -141,8 +141,19 @@ unsigned int chunked : 1; /* iff request body needs to be forwarded as chunked */ unsigned int serialize : 1; /* iff this request is written in HTTP/1.1 serialization */ apr_off_t raw_bytes; /* RAW network bytes that generated this request - if known. */ + int http_status; /* Store a possible HTTP status code that gets + * defined before creating the dummy HTTP/1.1 + * request e.g. due to an error already + * detected. + */ }; +/* + * A possible HTTP status code is not defined yet. See the http_status field + * in struct h2_request above for further explanation. + */ +#define H2_HTTP_STATUS_UNSET (0) + typedef struct h2_headers h2_headers; struct h2_headers { --- a/modules/http2/h2_bucket_beam.c +++ b/modules/http2/h2_bucket_beam.c @@ -945,7 +945,8 @@ apr_status_t h2_beam_receive(h2_bucket_beam *beam, apr_bucket_brigade *bb, apr_read_type_e block, - apr_off_t readbytes) + apr_off_t readbytes, + int *pclosed) { h2_beam_lock bl; apr_bucket *bsender, *brecv, *ng; @@ -953,7 +954,7 @@ apr_status_t status = APR_SUCCESS; apr_off_t remain; int transferred_buckets = 0; - + /* Called from the receiver thread to take buckets from the beam */ if (enter_yellow(beam, &bl) == APR_SUCCESS) { if (readbytes <= 0) { @@ -1039,6 +1040,7 @@ H2_BLIST_INSERT_TAIL(&beam->hold_list, bsender); remain -= bsender->length; + beam->received_bytes += bsender->length; ++transferred; ++transferred_buckets; continue; @@ -1126,7 +1128,8 @@ } goto transfer; } -leave: +leave: + if (pclosed) *pclosed = beam->closed? 1 : 0; leave_yellow(beam, &bl); } return status; --- a/modules/http2/h2_bucket_beam.h +++ b/modules/http2/h2_bucket_beam.h @@ -258,11 +258,15 @@ * if no data is available. * * Call from the receiver side only. + * @param pclosed on return != 0 iff the beam has been closed by the sender. It + * may still hold untransfered data. Maybe NULL if the caller is + * not interested in this. */ apr_status_t h2_beam_receive(h2_bucket_beam *beam, apr_bucket_brigade *green_buckets, apr_read_type_e block, - apr_off_t readbytes); + apr_off_t readbytes, + int *pclosed); /** * Determine if beam is empty. --- a/modules/http2/h2_config.c +++ b/modules/http2/h2_config.c @@ -78,6 +78,7 @@ int early_hints; /* support status code 103 */ int padding_bits; int padding_always; + int output_buffered; } h2_config; typedef struct h2_dir_config { @@ -115,6 +116,7 @@ 0, /* early hints, http status 103 */ 0, /* padding bits */ 1, /* padding always */ + 1, /* strean output buffered */ }; static h2_dir_config defdconf = { @@ -159,6 +161,7 @@ conf->early_hints = DEF_VAL; conf->padding_bits = DEF_VAL; conf->padding_always = DEF_VAL; + conf->output_buffered = DEF_VAL; return conf; } @@ -193,6 +196,7 @@ } n->push_diary_size = H2_CONFIG_GET(add, base, push_diary_size); n->copy_files = H2_CONFIG_GET(add, base, copy_files); + n->output_buffered = H2_CONFIG_GET(add, base, output_buffered); if (add->push_list && base->push_list) { n->push_list = apr_array_append(pool, base->push_list, add->push_list); } @@ -286,6 +290,8 @@ return H2_CONFIG_GET(conf, &defconf, padding_bits); case H2_CONF_PADDING_ALWAYS: return H2_CONFIG_GET(conf, &defconf, padding_always); + case H2_CONF_OUTPUT_BUFFER: + return H2_CONFIG_GET(conf, &defconf, output_buffered); default: return DEF_VAL; } @@ -351,6 +357,9 @@ case H2_CONF_PADDING_ALWAYS: H2_CONFIG_SET(conf, padding_always, val); break; + case H2_CONF_OUTPUT_BUFFER: + H2_CONFIG_SET(conf, output_buffered, val); + break; default: break; } @@ -721,7 +730,7 @@ else if (!strcasecmp("BEFORE", sdependency)) { dependency = H2_DEPENDANT_BEFORE; if (sweight) { - return "dependency 'Before' does not allow a weight"; + return "dependecy 'Before' does not allow a weight"; } } else if (!strcasecmp("INTERLEAVED", sdependency)) { @@ -904,6 +913,19 @@ return NULL; } +static const char *h2_conf_set_output_buffer(cmd_parms *cmd, + void *dirconf, const char *value) +{ + if (!strcasecmp(value, "On")) { + CONFIG_CMD_SET(cmd, dirconf, H2_CONF_OUTPUT_BUFFER, 1); + return NULL; + } + else if (!strcasecmp(value, "Off")) { + CONFIG_CMD_SET(cmd, dirconf, H2_CONF_OUTPUT_BUFFER, 0); + return NULL; + } + return "value must be On or Off"; +} void h2_get_num_workers(server_rec *s, int *minw, int *maxw) { @@ -975,6 +997,8 @@ RSRC_CONF, "on to enable interim status 103 responses"), AP_INIT_TAKE1("H2Padding", h2_conf_set_padding, NULL, RSRC_CONF, "set payload padding"), + AP_INIT_TAKE1("H2OutputBuffering", h2_conf_set_output_buffer, NULL, + RSRC_CONF, "set stream output buffer on/off"), AP_END_CMD }; --- a/modules/http2/h2_config.h +++ b/modules/http2/h2_config.h @@ -44,6 +44,7 @@ H2_CONF_EARLY_HINTS, H2_CONF_PADDING_BITS, H2_CONF_PADDING_ALWAYS, + H2_CONF_OUTPUT_BUFFER, } h2_config_var_t; struct apr_hash_t; --- a/modules/http2/h2_h2.c +++ b/modules/http2/h2_h2.c @@ -749,6 +749,7 @@ if (task) { /* check if we copy vs. setaside files in this location */ task->output.copy_files = h2_config_rgeti(r, H2_CONF_COPY_FILES); + task->output.buffered = h2_config_rgeti(r, H2_CONF_OUTPUT_BUFFER); if (task->output.copy_files) { ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, task->c, "h2_secondary_out(%s): copy_files on", task->id); --- a/modules/http2/h2_headers.c +++ b/modules/http2/h2_headers.c @@ -64,6 +64,7 @@ b = apr_bucket_shared_make(b, br, 0, 0); b->type = &h2_bucket_type_headers; + b->length = h2_headers_length(r); return b; } @@ -125,6 +126,20 @@ return headers; } +static int add_header_lengths(void *ctx, const char *name, const char *value) +{ + apr_size_t *plen = ctx; + *plen += strlen(name) + strlen(value); + return 1; +} + +apr_size_t h2_headers_length(h2_headers *headers) +{ + apr_size_t len = 0; + apr_table_do(add_header_lengths, &len, headers->headers, NULL); + return len; +} + h2_headers *h2_headers_rcreate(request_rec *r, int status, apr_table_t *header, apr_pool_t *pool) { --- a/modules/http2/h2_headers.h +++ b/modules/http2/h2_headers.h @@ -82,4 +82,9 @@ int h2_headers_are_response(h2_headers *headers); +/** + * Give the number of bytes of all contained header strings. + */ +apr_size_t h2_headers_length(h2_headers *headers); + #endif /* defined(__mod_h2__h2_headers__) */ --- a/modules/http2/h2_mplx.c +++ b/modules/http2/h2_mplx.c @@ -91,10 +91,6 @@ static void mst_check_data_for(h2_mplx *m, h2_stream *stream, int mplx_is_locked); -static void mst_stream_output_consumed(void *ctx, h2_bucket_beam *beam, apr_off_t length) -{ -} - static void mst_stream_input_ev(void *ctx, h2_bucket_beam *beam) { h2_stream *stream = ctx; @@ -299,18 +295,6 @@ stream->task = NULL; secondary = task->c; if (secondary) { - /* On non-serialized requests, the IO logging has not accounted for any - * meta data send over the network: response headers and h2 frame headers. we - * counted this on the stream and need to add this now. - * This is supposed to happen before the EOR bucket triggers the - * logging of the transaction. *fingers crossed* */ - if (task->request && !task->request->serialize && h2_task_logio_add_bytes_out) { - apr_off_t unaccounted = stream->out_frame_octets - stream->out_data_octets; - if (unaccounted > 0) { - h2_task_logio_add_bytes_out(secondary, unaccounted); - } - } - if (m->s->keep_alive_max == 0 || secondary->keepalives < m->s->keep_alive_max) { reuse_secondary = ((m->spare_secondary->nelts < (m->limit_active * 3 / 2)) && !task->rst_error); @@ -540,7 +524,6 @@ "h2_mplx(%s): out open", stream->task->id); } - h2_beam_on_consumed(stream->output, NULL, mst_stream_output_consumed, stream); h2_beam_on_produced(stream->output, mst_output_produced, stream); if (stream->task->output.copy_files) { h2_beam_on_file_beam(stream->output, h2_beam_no_files, NULL); --- a/modules/http2/h2_request.c +++ b/modules/http2/h2_request.c @@ -79,11 +79,12 @@ } req = apr_pcalloc(pool, sizeof(*req)); - req->method = apr_pstrdup(pool, r->method); - req->scheme = scheme; - req->authority = authority; - req->path = path; - req->headers = apr_table_make(pool, 10); + req->method = apr_pstrdup(pool, r->method); + req->scheme = scheme; + req->authority = authority; + req->path = path; + req->headers = apr_table_make(pool, 10); + req->http_status = H2_HTTP_STATUS_UNSET; if (r->server) { req->serialize = h2_config_rgeti(r, H2_CONF_SER_HEADERS); } @@ -269,9 +270,7 @@ request_rec *h2_request_create_rec(const h2_request *req, conn_rec *c) { - int access_status = HTTP_OK; - const char *rpath; - const char *s; + int access_status; #if AP_MODULE_MAGIC_AT_LEAST(20150222, 13) request_rec *r = ap_create_request(c); @@ -279,52 +278,88 @@ request_rec *r = my_ap_create_request(c); #endif - r->headers_in = apr_table_clone(r->pool, req->headers); - +#if AP_MODULE_MAGIC_AT_LEAST(20200331, 3) ap_run_pre_read_request(r, c); - + /* Time to populate r with the data we have. */ r->request_time = req->request_time; - r->method = apr_pstrdup(r->pool, req->method); - /* Provide quick information about the request method as soon as known */ - r->method_number = ap_method_number_of(r->method); - if (r->method_number == M_GET && r->method[0] == 'H') { - r->header_only = 1; - } - r->the_request = apr_psprintf(r->pool, "%s %s HTTP/2.0", + r->the_request = apr_psprintf(r->pool, "%s %s HTTP/2.0", req->method, req->path ? req->path : ""); r->headers_in = apr_table_clone(r->pool, req->headers); - rpath = (req->path ? req->path : ""); - ap_parse_uri(r, rpath); - r->protocol = (char*)"HTTP/2.0"; - r->proto_num = HTTP_VERSION(2, 0); - - r->the_request = apr_psprintf(r->pool, "%s %s %s", - r->method, rpath, r->protocol); - - /* update what we think the virtual host is based on the headers we've - * now read. may update status. - * Leave r->hostname empty, vhost will parse if form our Host: header, - * otherwise we get complains about port numbers. + /* Start with r->hostname = NULL, ap_check_request_header() will get it + * form Host: header, otherwise we get complains about port numbers. */ r->hostname = NULL; - ap_update_vhost_from_headers(r); - r->protocol = "HTTP/2.0"; - r->proto_num = HTTP_VERSION(2, 0); - /* we may have switched to another server */ - r->per_dir_config = r->server->lookup_defaults; - - s = apr_table_get(r->headers_in, "Expect"); - if (s && s[0]) { - if (ap_cstr_casecmp(s, "100-continue") == 0) { - r->expecting_100 = 1; + /* Validate HTTP/1 request and select vhost. */ + if (!ap_parse_request_line(r) || !ap_check_request_header(r)) { + /* we may have switched to another server still */ + r->per_dir_config = r->server->lookup_defaults; + if (req->http_status != H2_HTTP_STATUS_UNSET) { + access_status = req->http_status; + /* Be safe and close the connection */ + c->keepalive = AP_CONN_CLOSE; } else { - r->status = HTTP_EXPECTATION_FAILED; - ap_send_error_response(r, 0); + access_status = r->status; } + r->status = HTTP_OK; + goto die; + } +#else + { + const char *s; + + r->headers_in = apr_table_clone(r->pool, req->headers); + ap_run_pre_read_request(r, c); + + /* Time to populate r with the data we have. */ + r->request_time = req->request_time; + r->method = apr_pstrdup(r->pool, req->method); + /* Provide quick information about the request method as soon as known */ + r->method_number = ap_method_number_of(r->method); + if (r->method_number == M_GET && r->method[0] == 'H') { + r->header_only = 1; + } + ap_parse_uri(r, req->path ? req->path : ""); + r->protocol = (char*)"HTTP/2.0"; + r->proto_num = HTTP_VERSION(2, 0); + r->the_request = apr_psprintf(r->pool, "%s %s HTTP/2.0", + r->method, req->path ? req->path : ""); + + /* Start with r->hostname = NULL, ap_check_request_header() will get it + * form Host: header, otherwise we get complains about port numbers. + */ + r->hostname = NULL; + ap_update_vhost_from_headers(r); + + /* we may have switched to another server */ + r->per_dir_config = r->server->lookup_defaults; + + s = apr_table_get(r->headers_in, "Expect"); + if (s && s[0]) { + if (ap_cstr_casecmp(s, "100-continue") == 0) { + r->expecting_100 = 1; + } + else { + r->status = HTTP_EXPECTATION_FAILED; + access_status = r->status; + goto die; + } + } + } +#endif + + /* we may have switched to another server */ + r->per_dir_config = r->server->lookup_defaults; + + if (req->http_status != H2_HTTP_STATUS_UNSET) { + access_status = req->http_status; + r->status = HTTP_OK; + /* Be safe and close the connection */ + c->keepalive = AP_CONN_CLOSE; + goto die; } /* @@ -336,28 +371,47 @@ ap_add_input_filter_handle(ap_http_input_filter_handle, NULL, r, r->connection); - if (access_status != HTTP_OK - || (access_status = ap_run_post_read_request(r))) { + if ((access_status = ap_run_post_read_request(r))) { /* Request check post hooks failed. An example of this would be a * request for a vhost where h2 is disabled --> 421. */ ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, c, APLOGNO(03367) "h2_request: access_status=%d, request_create failed", access_status); - ap_die(access_status, r); - ap_update_child_status(c->sbh, SERVER_BUSY_LOG, r); - ap_run_log_transaction(r); - r = NULL; - goto traceout; + goto die; } AP_READ_REQUEST_SUCCESS((uintptr_t)r, (char *)r->method, (char *)r->uri, (char *)r->server->defn_name, r->status); return r; -traceout: + +die: + ap_die(access_status, r); + + /* ap_die() sent the response through the output filters, we must now + * end the request with an EOR bucket for stream/pipeline accounting. + */ + { + apr_bucket_brigade *eor_bb; +#if AP_MODULE_MAGIC_AT_LEAST(20180905, 1) + eor_bb = ap_acquire_brigade(c); + APR_BRIGADE_INSERT_TAIL(eor_bb, + ap_bucket_eor_create(c->bucket_alloc, r)); + ap_pass_brigade(c->output_filters, eor_bb); + ap_release_brigade(c, eor_bb); +#else + eor_bb = apr_brigade_create(c->pool, c->bucket_alloc); + APR_BRIGADE_INSERT_TAIL(eor_bb, + ap_bucket_eor_create(c->bucket_alloc, r)); + ap_pass_brigade(c->output_filters, eor_bb); + apr_brigade_destroy(eor_bb); +#endif + } + + r = NULL; AP_READ_REQUEST_FAILURE((uintptr_t)r); - return r; + return NULL; } --- a/modules/http2/h2_session.c +++ b/modules/http2/h2_session.c @@ -311,7 +311,9 @@ status = h2_stream_add_header(stream, (const char *)name, namelen, (const char *)value, valuelen); - if (status != APR_SUCCESS && !h2_stream_is_ready(stream)) { + if (status != APR_SUCCESS + && (!stream->rtmp + || stream->rtmp->http_status == H2_HTTP_STATUS_UNSET)) { return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE; } return 0; --- a/modules/http2/h2_stream.c +++ b/modules/http2/h2_stream.c @@ -639,16 +639,7 @@ static void set_error_response(h2_stream *stream, int http_status) { if (!h2_stream_is_ready(stream)) { - conn_rec *c = stream->session->c; - apr_bucket *b; - h2_headers *response; - - response = h2_headers_die(http_status, stream->request, stream->pool); - prep_output(stream); - b = apr_bucket_eos_create(c->bucket_alloc); - APR_BRIGADE_INSERT_HEAD(stream->out_buffer, b); - b = h2_bucket_headers_create(c->bucket_alloc, response); - APR_BRIGADE_INSERT_HEAD(stream->out_buffer, b); + stream->rtmp->http_status = http_status; } } @@ -910,7 +901,7 @@ apr_status_t status = APR_SUCCESS; apr_off_t requested, missing, max_chunk = H2_DATA_CHUNK_SIZE; conn_rec *c; - int complete; + int complete, was_closed = 0; ap_assert(stream); @@ -959,9 +950,11 @@ if (stream->output) { H2_STREAM_OUT_LOG(APLOG_TRACE2, stream, "pre"); - rv = h2_beam_receive(stream->output, stream->out_buffer, - APR_NONBLOCK_READ, stream->max_mem - *plen); + h2_beam_log(stream->output, c, APLOG_TRACE2, "pre read output"); + rv = h2_beam_receive(stream->output, stream->out_buffer, + APR_NONBLOCK_READ, stream->max_mem - *plen, &was_closed); H2_STREAM_OUT_LOG(APLOG_TRACE2, stream, "post"); + h2_beam_log(stream->output, c, APLOG_TRACE2, "post read output"); } if (rv == APR_SUCCESS) { @@ -991,7 +984,7 @@ (long)*plen, *peos); } else { - status = (stream->output && h2_beam_is_closed(stream->output))? APR_EOF : APR_EAGAIN; + status = was_closed? APR_EOF : APR_EAGAIN; ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, c, H2_STRM_MSG(stream, "prepare, no data")); } --- a/modules/http2/h2_stream.h +++ b/modules/http2/h2_stream.h @@ -92,7 +92,8 @@ unsigned int input_eof : 1; /* no more request data coming */ unsigned int out_checked : 1; /* output eof was double checked */ unsigned int push_policy; /* which push policy to use for this request */ - + unsigned int input_buffering : 1; /* buffer request bodies for efficiency */ + struct h2_task *task; /* assigned task to fullfill request */ const h2_priority *pref_priority; /* preferred priority for this stream */ --- a/modules/http2/h2_task.c +++ b/modules/http2/h2_task.c @@ -89,6 +89,14 @@ return h2_mplx_t_out_open(task->mplx, task->stream_id, task->output.beam); } +static void output_consumed(void *ctx, h2_bucket_beam *beam, apr_off_t length) +{ + h2_task *task = ctx; + if (task && h2_task_logio_add_bytes_out) { + h2_task_logio_add_bytes_out(task->c, length); + } +} + static apr_status_t send_out(h2_task *task, apr_bucket_brigade* bb, int block) { apr_off_t written, left; @@ -108,9 +116,6 @@ status = APR_SUCCESS; } if (status == APR_SUCCESS) { - if (h2_task_logio_add_bytes_out) { - h2_task_logio_add_bytes_out(task->c, written); - } ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, task->c, "h2_task(%s): send_out done", task->id); } @@ -183,7 +188,9 @@ } } - if (APR_SUCCESS == rv && !task->output.opened && flush) { + ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, task->c, + "h2_secondary_out(%s): buffered=%d", task->id, task->output.buffered); + if (APR_SUCCESS == rv && !task->output.opened && (flush || !task->output.buffered)) { /* got a flush or could not write all, time to tell someone to read */ rv = open_output(task); } @@ -259,7 +266,7 @@ } if (task->input.beam) { status = h2_beam_receive(task->input.beam, task->input.bb, block, - 128*1024); + 128*1024, NULL); } else { status = APR_EOF; @@ -597,7 +604,8 @@ h2_beam_buffer_size_set(task->output.beam, task->output.max_buffer); h2_beam_send_from(task->output.beam, task->pool); - + h2_beam_on_consumed(task->output.beam, NULL, output_consumed, task); + h2_ctx_create_for(c, task); apr_table_setn(c->notes, H2_TASK_ID_NOTE, task->id); --- a/modules/http2/h2_task.h +++ b/modules/http2/h2_task.h @@ -71,6 +71,7 @@ unsigned int opened : 1; unsigned int sent_response : 1; unsigned int copy_files : 1; + unsigned int buffered : 1; struct h2_response_parser *rparser; apr_bucket_brigade *bb; apr_size_t max_buffer; --- a/modules/http2/h2_version.h +++ b/modules/http2/h2_version.h @@ -27,7 +27,7 @@ * @macro * Version number of the http2 module as c string */ -#define MOD_HTTP2_VERSION "1.15.14" +#define MOD_HTTP2_VERSION "1.15.17" /** * @macro @@ -35,6 +35,7 @@ * release. This is a 24 bit number with 8 bits for major number, 8 bits * for minor and 8 bits for patch. Version 1.2.3 becomes 0x010203. */ -#define MOD_HTTP2_VERSION_NUM 0x010f0e +#define MOD_HTTP2_VERSION_NUM 0x010f11 + #endif /* mod_h2_h2_version_h */ --- a/modules/http2/h2_workers.c +++ b/modules/http2/h2_workers.c @@ -34,17 +34,16 @@ typedef struct h2_slot h2_slot; struct h2_slot { int id; + int sticks; h2_slot *next; h2_workers *workers; - int aborted; - int sticks; h2_task *task; apr_thread_t *thread; apr_thread_mutex_t *lock; apr_thread_cond_t *not_idle; }; -static h2_slot *pop_slot(h2_slot **phead) +static h2_slot *pop_slot(h2_slot *volatile *phead) { /* Atomically pop a slot from the list */ for (;;) { @@ -59,7 +58,7 @@ } } -static void push_slot(h2_slot **phead, h2_slot *slot) +static void push_slot(h2_slot *volatile *phead, h2_slot *slot) { /* Atomically push a slot to the list */ ap_assert(!slot->next); @@ -78,7 +77,6 @@ apr_status_t status; slot->workers = workers; - slot->aborted = 0; slot->task = NULL; if (!slot->lock) { @@ -101,16 +99,18 @@ ap_log_error(APLOG_MARK, APLOG_TRACE2, 0, workers->s, "h2_workers: new thread for slot %d", slot->id); + /* thread will either immediately start work or add itself * to the idle queue */ - apr_thread_create(&slot->thread, workers->thread_attr, slot_run, slot, - workers->pool); - if (!slot->thread) { + apr_atomic_inc32(&workers->worker_count); + status = apr_thread_create(&slot->thread, workers->thread_attr, + slot_run, slot, workers->pool); + if (status != APR_SUCCESS) { + apr_atomic_dec32(&workers->worker_count); push_slot(&workers->free, slot); - return APR_ENOMEM; + return status; } - apr_atomic_inc32(&workers->worker_count); return APR_SUCCESS; } @@ -136,17 +136,15 @@ } } -static void cleanup_zombies(h2_workers *workers) +static void join_zombies(h2_workers *workers) { h2_slot *slot; while ((slot = pop_slot(&workers->zombies))) { - if (slot->thread) { - apr_status_t status; - apr_thread_join(&status, slot->thread); - slot->thread = NULL; - } - apr_atomic_dec32(&workers->worker_count); - slot->next = NULL; + apr_status_t status; + ap_assert(slot->thread != NULL); + apr_thread_join(&status, slot->thread); + slot->thread = NULL; + push_slot(&workers->free, slot); } } @@ -184,37 +182,49 @@ * Get the next task for the given worker. Will block until a task arrives * or the max_wait timer expires and more than min workers exist. */ -static apr_status_t get_next(h2_slot *slot) +static int get_next(h2_slot *slot) { h2_workers *workers = slot->workers; - apr_status_t status; - - slot->task = NULL; - while (!slot->aborted) { - if (!slot->task) { - status = h2_fifo_try_peek(workers->mplxs, mplx_peek, slot); - if (status == APR_EOF) { - return status; - } + + while (!workers->aborted) { + ap_assert(slot->task == NULL); + if (h2_fifo_try_peek(workers->mplxs, mplx_peek, slot) == APR_EOF) { + /* The queue is terminated with the MPM child being cleaned up, + * just leave. + */ + break; } - if (slot->task) { - return APR_SUCCESS; + return 1; } - cleanup_zombies(workers); + join_zombies(workers); apr_thread_mutex_lock(slot->lock); - push_slot(&workers->idle, slot); - apr_thread_cond_wait(slot->not_idle, slot->lock); + if (!workers->aborted) { + push_slot(&workers->idle, slot); + apr_thread_cond_wait(slot->not_idle, slot->lock); + } apr_thread_mutex_unlock(slot->lock); } - return APR_EOF; + + return 0; } static void slot_done(h2_slot *slot) { - push_slot(&(slot->workers->zombies), slot); + h2_workers *workers = slot->workers; + + push_slot(&workers->zombies, slot); + + /* If this worker is the last one exiting and the MPM child is stopping, + * unblock workers_pool_cleanup(). + */ + if (!apr_atomic_dec32(&workers->worker_count) && workers->aborted) { + apr_thread_mutex_lock(workers->lock); + apr_thread_cond_signal(workers->all_done); + apr_thread_mutex_unlock(workers->lock); + } } @@ -222,28 +232,28 @@ { h2_slot *slot = wctx; - while (!slot->aborted) { - - /* Get a h2_task from the mplxs queue. */ - get_next(slot); - while (slot->task) { - + /* Get the h2_task(s) from the ->mplxs queue. */ + while (get_next(slot)) { + ap_assert(slot->task != NULL); + do { h2_task_do(slot->task, thread, slot->id); /* Report the task as done. If stickyness is left, offer the * mplx the opportunity to give us back a new task right away. */ - if (!slot->aborted && (--slot->sticks > 0)) { + if (!slot->workers->aborted && --slot->sticks > 0) { h2_mplx_s_task_done(slot->task->mplx, slot->task, &slot->task); } else { h2_mplx_s_task_done(slot->task->mplx, slot->task, NULL); slot->task = NULL; } - } + } while (slot->task); } slot_done(slot); + + apr_thread_exit(thread, APR_SUCCESS); return NULL; } @@ -252,30 +262,28 @@ h2_workers *workers = data; h2_slot *slot; - if (!workers->aborted) { - workers->aborted = 1; - /* abort all idle slots */ - for (;;) { - slot = pop_slot(&workers->idle); - if (slot) { - apr_thread_mutex_lock(slot->lock); - slot->aborted = 1; - apr_thread_cond_signal(slot->not_idle); - apr_thread_mutex_unlock(slot->lock); - } - else { - break; - } - } + workers->aborted = 1; + h2_fifo_term(workers->mplxs); - h2_fifo_term(workers->mplxs); + /* abort all idle slots */ + while ((slot = pop_slot(&workers->idle))) { + apr_thread_mutex_lock(slot->lock); + apr_thread_cond_signal(slot->not_idle); + apr_thread_mutex_unlock(slot->lock); + } - cleanup_zombies(workers); + /* wait for all the workers to become zombies and join them */ + apr_thread_mutex_lock(workers->lock); + if (apr_atomic_read32(&workers->worker_count)) { + apr_thread_cond_wait(workers->all_done, workers->lock); } + apr_thread_mutex_unlock(workers->lock); + join_zombies(workers); + return APR_SUCCESS; } -h2_workers *h2_workers_create(server_rec *s, apr_pool_t *server_pool, +h2_workers *h2_workers_create(server_rec *s, apr_pool_t *pchild, int min_workers, int max_workers, int idle_secs) { @@ -285,14 +293,14 @@ int i, n; ap_assert(s); - ap_assert(server_pool); + ap_assert(pchild); /* let's have our own pool that will be parent to all h2_worker * instances we create. This happens in various threads, but always * guarded by our lock. Without this pool, all subpool creations would * happen on the pool handed to us, which we do not guard. */ - apr_pool_create(&pool, server_pool); + apr_pool_create(&pool, pchild); apr_pool_tag(pool, "h2_workers"); workers = apr_pcalloc(pool, sizeof(h2_workers)); if (!workers) { @@ -338,6 +346,9 @@ APR_THREAD_MUTEX_DEFAULT, workers->pool); if (status == APR_SUCCESS) { + status = apr_thread_cond_create(&workers->all_done, workers->pool); + } + if (status == APR_SUCCESS) { n = workers->nslots = workers->max_workers; workers->slots = apr_pcalloc(workers->pool, n * sizeof(h2_slot)); if (workers->slots == NULL) { @@ -363,7 +374,12 @@ workers->dynamic = (workers->worker_count < workers->max_workers); } if (status == APR_SUCCESS) { - apr_pool_pre_cleanup_register(pool, workers, workers_pool_cleanup); + /* Stop/join the workers threads when the MPM child exits (pchild is + * destroyed), and as a pre_cleanup of pchild thus before the threads + * pools (children of workers->pool) so that they are not destroyed + * before/under us. + */ + apr_pool_pre_cleanup_register(pchild, workers, workers_pool_cleanup); return workers; } return NULL; --- a/modules/http2/h2_workers.h +++ b/modules/http2/h2_workers.h @@ -42,7 +42,7 @@ int max_workers; int max_idle_secs; - int aborted; + volatile int aborted; int dynamic; apr_threadattr_t *thread_attr; @@ -58,6 +58,7 @@ struct h2_fifo *mplxs; struct apr_thread_mutex_t *lock; + struct apr_thread_cond_t *all_done; }; --- a/modules/http2/mod_http2.c +++ b/modules/http2/mod_http2.c @@ -180,15 +180,33 @@ /* Runs once per created child process. Perform any process * related initionalization here. */ -static void h2_child_init(apr_pool_t *pool, server_rec *s) +static void h2_child_init(apr_pool_t *pchild, server_rec *s) { + apr_allocator_t *allocator; + apr_thread_mutex_t *mutex; + apr_status_t status; + + /* The allocator of pchild has no mutex with MPM prefork, but we need one + * for h2 workers threads synchronization. Even though mod_http2 shouldn't + * be used with prefork, better be safe than sorry, so forcibly set the + * mutex here. For MPM event/worker, pchild has no allocator so pconf's + * is used, with its mutex. + */ + allocator = apr_pool_allocator_get(pchild); + if (allocator) { + mutex = apr_allocator_mutex_get(allocator); + if (!mutex) { + apr_thread_mutex_create(&mutex, APR_THREAD_MUTEX_DEFAULT, pchild); + apr_allocator_mutex_set(allocator, mutex); + } + } + /* Set up our connection processing */ - apr_status_t status = h2_conn_child_init(pool, s); + status = h2_conn_child_init(pchild, s); if (status != APR_SUCCESS) { ap_log_error(APLOG_MARK, APLOG_ERR, status, s, APLOGNO(02949) "initializing connection handling"); } - } /* Install this module into the apache2 infrastructure. --- a/modules/http2/mod_proxy_http2.c +++ b/modules/http2/mod_proxy_http2.c @@ -425,7 +425,7 @@ ctx->p_conn = NULL; } ++reconnects; - if (reconnects < 5) { + if (reconnects < 2) { goto run_connect; } ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, ctx->owner, APLOGNO(10023)