From: Stefan Eissing Date: Mon, 2 Mar 2020 10:42:30 +0000 (+0000) Subject: *) mod_http2: Fixes issue where mod_unique_id would generate non-unique request X-Git-Tag: 2.5.0-alpha2-ci-test-only~1601 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=9020a5f8a1d620df6a024ab1309fe8e1267931f5;p=thirdparty%2Fapache%2Fhttpd.git *) mod_http2: Fixes issue where mod_unique_id would generate non-unique request identifier under load, see . [Michael Kaufmann, Stefan Eissing] git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1874689 13f79535-47bb-0310-9956-ffa450edef68 --- diff --git a/CHANGES b/CHANGES index 8786b51d51c..e41041f2e37 100644 --- a/CHANGES +++ b/CHANGES @@ -1,6 +1,10 @@ -*- coding: utf-8 -*- Changes with Apache 2.5.1 + *) mod_http2: Fixes issue where mod_unique_id would generate non-unique request + identifier under load, see . + [Michael Kaufmann, Stefan Eissing] + *) mod_session_cookie: Add SessionCookieMaxAge to allow the mod_session cookie to be sent as a "session cookie" with no expiration even when the SessionMaxAge will be enforced on the server. PR56040 [Eric Covener] diff --git a/modules/http2/h2_bucket_beam.c b/modules/http2/h2_bucket_beam.c index a7f5edf5cc5..607ac1e3824 100644 --- a/modules/http2/h2_bucket_beam.c +++ b/modules/http2/h2_bucket_beam.c @@ -196,9 +196,8 @@ 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) { - (void)ctx; apr_thread_mutex_unlock(lock); } @@ -219,7 +218,7 @@ static void leave_yellow(h2_bucket_beam *beam, h2_beam_lock *pbl) { (void)beam; 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 72bbc2432c3..c3bf6a6db0e 100644 --- a/modules/http2/h2_task.c +++ b/modules/http2/h2_task.c @@ -556,37 +556,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 9c318c0d57a..98738db7ea0 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 0b73b3b46cc..3770b38ece2 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,7 +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 0x010f05 +#define MOD_HTTP2_VERSION_NUM 0x010f08 #endif /* mod_h2_h2_version_h */