From: Jim Jagielski Date: Fri, 6 Mar 2020 16:15:17 +0000 (+0000) Subject: Merge r1874689 from trunk: X-Git-Tag: 2.4.42~15 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=1dd9ab5701da7b63be488df9c87dcaf7e2704861;p=thirdparty%2Fapache%2Fhttpd.git Merge r1874689 from trunk: *) mod_http2: Fixes issue where mod_unique_id would generate non-unique request identifier under load, see . [Michael Kaufmann, Stefan Eissing] Submitted by: icing Reviewed by: icing, ylavic, jim git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.4.x@1874909 13f79535-47bb-0310-9956-ffa450edef68 --- diff --git a/CHANGES b/CHANGES index cb7b647ef30..2381c9077ca 100644 --- a/CHANGES +++ b/CHANGES @@ -1,6 +1,10 @@ -*- coding: utf-8 -*- Changes with Apache 2.4.42 + *) mod_http2: Fixes issue where mod_unique_id would generate non-unique request + identifier under load, see . + [Michael Kaufmann, Stefan Eissing] + *) mod_proxy_hcheck: Allow healthcheck expressions to use %{Content-Type}. PR64140. [Renier Velazco ] diff --git a/STATUS b/STATUS index b069466ee1c..e4850453e1b 100644 --- a/STATUS +++ b/STATUS @@ -146,12 +146,6 @@ PATCHES ACCEPTED TO BACKPORT FROM TRUNK: 2.4.x patch: svn merge -c 1874323 ^/httpd/httpd/trunk . +1: ylavic, gsmith, rpluem - *) mod_http2: Fixes issue where mod_unique_id would generate non-unique request - identifier under load, see . - trunk patch: http://svn.apache.org/r1874689 - 2.4.x patch: svn merge -c 1874689 ^/httpd/httpd/trunk . - +1: icing, ylavic, jim - PATCHES PROPOSED TO BACKPORT FROM TRUNK: [ New proposals should be added at the end of the list ] diff --git a/modules/http2/h2_bucket_beam.c b/modules/http2/h2_bucket_beam.c index f79cbe33566..17976f2d14b 100644 --- a/modules/http2/h2_bucket_beam.c +++ b/modules/http2/h2_bucket_beam.c @@ -196,7 +196,7 @@ static apr_bucket *h2_beam_bucket(h2_bucket_beam *beam, * bucket beam that can transport buckets across threads ******************************************************************************/ -static void mutex_leave(void *ctx, apr_thread_mutex_t *lock) +static void mutex_leave(apr_thread_mutex_t *lock) { apr_thread_mutex_unlock(lock); } @@ -217,7 +217,7 @@ static apr_status_t enter_yellow(h2_bucket_beam *beam, h2_beam_lock *pbl) static void leave_yellow(h2_bucket_beam *beam, h2_beam_lock *pbl) { if (pbl->leave) { - pbl->leave(pbl->leave_ctx, pbl->mutex); + pbl->leave(pbl->mutex); } } diff --git a/modules/http2/h2_bucket_beam.h b/modules/http2/h2_bucket_beam.h index f260762366e..1a6418cefc3 100644 --- a/modules/http2/h2_bucket_beam.h +++ b/modules/http2/h2_bucket_beam.h @@ -126,12 +126,11 @@ typedef struct { * buffers until the transmission is complete. Star gates use a similar trick. */ -typedef void h2_beam_mutex_leave(void *ctx, struct apr_thread_mutex_t *lock); +typedef void h2_beam_mutex_leave(struct apr_thread_mutex_t *lock); typedef struct { apr_thread_mutex_t *mutex; h2_beam_mutex_leave *leave; - void *leave_ctx; } h2_beam_lock; typedef struct h2_bucket_beam h2_bucket_beam; diff --git a/modules/http2/h2_task.c b/modules/http2/h2_task.c index bf29455f069..d6108957717 100644 --- a/modules/http2/h2_task.c +++ b/modules/http2/h2_task.c @@ -555,37 +555,36 @@ apr_status_t h2_task_do(h2_task *task, apr_thread_t *thread, int worker_id) task->worker_started = 1; if (c->master) { - /* Each conn_rec->id is supposed to be unique at a point in time. Since + /* See the discussion at + * + * Each conn_rec->id is supposed to be unique at a point in time. Since * some modules (and maybe external code) uses this id as an identifier * for the request_rec they handle, it needs to be unique for slave * connections also. - * The connection id is generated by the MPM and most MPMs use the formula - * id := (child_num * max_threads) + thread_num - * which means that there is a maximum id of about - * idmax := max_child_count * max_threads - * If we assume 2024 child processes with 2048 threads max, we get - * idmax ~= 2024 * 2048 = 2 ** 22 - * On 32 bit systems, we have not much space left, but on 64 bit systems - * (and higher?) we can use the upper 32 bits without fear of collision. - * 32 bits is just what we need, since a connection can only handle so - * many streams. + * + * The MPM module assigns the connection ids and mod_unique_id is using + * that one to generate identifier for requests. While the implementation + * works for HTTP/1.x, the parallel execution of several requests per + * connection will generate duplicate identifiers on load. + * + * The original implementation for slave connection identifiers used + * to shift the master connection id up and assign the stream id to the + * lower bits. This was cramped on 32 bit systems, but on 64bit there was + * enough space. + * + * As issue 195 showed, mod_unique_id only uses the lower 32 bit of the + * connection id, even on 64bit systems. Therefore collisions in request ids. + * + * The way master connection ids are generated, there is some space "at the + * top" of the lower 32 bits on allmost all systems. If you have a setup + * with 64k threads per child and 255 child processes, you live on the edge. + * + * The new implementation shifts 8 bits and XORs in the worker + * id. This will experience collisions with > 256 h2 workers and heavy + * load still. There seems to be no way to solve this in all possible + * configurations by mod_h2 alone. */ - int slave_id, free_bits; - - task->id = apr_psprintf(task->pool, "%ld-%d", c->master->id, - task->stream_id); - if (sizeof(unsigned long) >= 8) { - free_bits = 32; - slave_id = task->stream_id; - } - else { - /* Assume we have a more limited number of threads/processes - * and h2 workers on a 32-bit system. Use the worker instead - * of the stream id. */ - free_bits = 8; - slave_id = worker_id; - } - task->c->id = (c->master->id << free_bits)^slave_id; + task->c->id = (c->master->id << 8)^worker_id; } h2_beam_create(&task->output.beam, c->pool, task->stream_id, "output", diff --git a/modules/http2/h2_util.c b/modules/http2/h2_util.c index 1e093e9d963..dd758f3d01b 100644 --- a/modules/http2/h2_util.c +++ b/modules/http2/h2_util.c @@ -1923,7 +1923,8 @@ int h2_util_frame_print(const nghttp2_frame *frame, char *buffer, size_t maxlen) case NGHTTP2_GOAWAY: { size_t len = (frame->goaway.opaque_data_len < s_len)? frame->goaway.opaque_data_len : s_len-1; - memcpy(scratch, frame->goaway.opaque_data, len); + if (len) + memcpy(scratch, frame->goaway.opaque_data, len); scratch[len] = '\0'; return apr_snprintf(buffer, maxlen, "GOAWAY[error=%d, reason='%s', " "last_stream=%d]", frame->goaway.error_code, diff --git a/modules/http2/h2_version.h b/modules/http2/h2_version.h index 1254e912419..787551cba17 100644 --- 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.5" +#define MOD_HTTP2_VERSION "1.15.8" /** * @macro @@ -35,6 +35,6 @@ * 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 0x010f05 +#define MOD_HTTP2_VERSION_NUM 0x010f08 #endif /* mod_h2_h2_version_h */