diff --git a/ChangeLog b/ChangeLog index 1f6739c0..cbd9810e 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,8 @@ +v1.10.12 +-------------------------------------------------------------------------------- + * Reverting 1.10.11 change since showed no effect on issue #120 + * Fixes a race condition on request body handling. + v1.10.11 -------------------------------------------------------------------------------- * DoS flow control protection is less agressive as long as active tasks stay diff --git a/configure.ac b/configure.ac index abadf374..ecd7ba37 100644 --- a/configure.ac +++ b/configure.ac @@ -14,7 +14,7 @@ # AC_PREREQ([2.69]) -AC_INIT([mod_http2], [1.10.11], [stefan.eissing@greenbytes.de]) +AC_INIT([mod_http2], [1.10.12], [stefan.eissing@greenbytes.de]) LT_PREREQ([2.2.6]) LT_INIT() diff --git a/mod_http2/h2_bucket_beam.c b/mod_http2/h2_bucket_beam.c index 9f3bc709..6b8c645b 100644 --- a/mod_http2/h2_bucket_beam.c +++ b/mod_http2/h2_bucket_beam.c @@ -210,16 +210,7 @@ static apr_status_t mutex_enter(void *ctx, h2_beam_lock *pbl) static apr_status_t enter_yellow(h2_bucket_beam *beam, h2_beam_lock *pbl) { - h2_beam_mutex_enter *enter = beam->m_enter; - if (enter) { - void *ctx = beam->m_ctx; - if (ctx) { - return enter(ctx, pbl); - } - } - pbl->mutex = NULL; - pbl->leave = NULL; - return APR_SUCCESS; + return mutex_enter(beam, pbl); } static void leave_yellow(h2_bucket_beam *beam, h2_beam_lock *pbl) @@ -568,13 +559,12 @@ static apr_status_t beam_cleanup(void *data) { h2_bucket_beam *beam = data; apr_status_t status = APR_SUCCESS; - int safe_send = !beam->m_enter || (beam->owner == H2_BEAM_OWNER_SEND); - int safe_recv = !beam->m_enter || (beam->owner == H2_BEAM_OWNER_RECV); + int safe_send = (beam->owner == H2_BEAM_OWNER_SEND); + int safe_recv = (beam->owner == H2_BEAM_OWNER_RECV); /* * Owner of the beam is going away, depending on which side it owns, - * cleanup strategies will differ with multi-thread protection - * still in place (beam->m_enter). + * cleanup strategies will differ. * * In general, receiver holds references to memory from sender. * Clean up receiver first, if safe, then cleanup sender, if safe. @@ -680,29 +670,6 @@ apr_size_t h2_beam_buffer_size_get(h2_bucket_beam *beam) return buffer_size; } -void h2_beam_mutex_set(h2_bucket_beam *beam, - h2_beam_mutex_enter m_enter, - void *m_ctx) -{ - h2_beam_lock bl; - - if (enter_yellow(beam, &bl) == APR_SUCCESS) { - beam->m_enter = m_enter; - beam->m_ctx = m_ctx; - leave_yellow(beam, &bl); - } -} - -void h2_beam_mutex_enable(h2_bucket_beam *beam) -{ - h2_beam_mutex_set(beam, mutex_enter, beam); -} - -void h2_beam_mutex_disable(h2_bucket_beam *beam) -{ - h2_beam_mutex_set(beam, NULL, NULL); -} - void h2_beam_timeout_set(h2_bucket_beam *beam, apr_interval_time_t timeout) { h2_beam_lock bl; diff --git a/mod_http2/h2_bucket_beam.h b/mod_http2/h2_bucket_beam.h index 7a67455b..c48d1538 100644 --- a/mod_http2/h2_bucket_beam.h +++ b/mod_http2/h2_bucket_beam.h @@ -64,13 +64,11 @@ typedef struct { * via the h2_beam_send(). It gives the beam to the green thread which then * can receive buckets into its own brigade via h2_beam_receive(). * - * Sending and receiving can happen concurrently, if a thread mutex is set - * for the beam, see h2_beam_mutex_set. + * Sending and receiving can happen concurrently. * * The beam can limit the amount of data it accepts via the buffer_size. This - * can also be adjusted during its lifetime. When the beam not only gets a - * mutex but als a condition variable (in h2_beam_mutex_set()), sends and - * receives can be done blocking. A timeout can be set for such blocks. + * can also be adjusted during its lifetime. Sends and receives can be done blocking. + * A timeout can be set for such blocks. * * Care needs to be taken when terminating the beam. The beam registers at * the pool it was created with and will cleanup after itself. However, if @@ -191,8 +189,6 @@ struct h2_bucket_beam { struct apr_thread_mutex_t *lock; struct apr_thread_cond_t *change; - void *m_ctx; - h2_beam_mutex_enter *m_enter; apr_off_t cons_bytes_reported; /* amount of bytes reported as consumed */ h2_beam_ev_callback *cons_ev_cb; @@ -315,13 +311,6 @@ int h2_beam_is_closed(h2_bucket_beam *beam); */ apr_status_t h2_beam_wait_empty(h2_bucket_beam *beam, apr_read_type_e block); -void h2_beam_mutex_set(h2_bucket_beam *beam, - h2_beam_mutex_enter m_enter, - void *m_ctx); - -void h2_beam_mutex_enable(h2_bucket_beam *beam); -void h2_beam_mutex_disable(h2_bucket_beam *beam); - /** * Set/get the timeout for blocking read/write operations. Only works * if a mutex has been set for the beam. diff --git a/mod_http2/h2_mplx.c b/mod_http2/h2_mplx.c index 51483adb..fe73cce3 100644 --- a/mod_http2/h2_mplx.c +++ b/mod_http2/h2_mplx.c @@ -543,9 +543,6 @@ static apr_status_t out_open(h2_mplx *m, int stream_id, h2_bucket_beam *beam) h2_beam_on_file_beam(stream->output, h2_beam_no_files, NULL); } - /* time to protect the beam against multi-threaded use */ - h2_beam_mutex_enable(stream->output); - /* we might see some file buckets in the output, see * if we have enough handles reserved. */ check_data_for(m, stream, 0); @@ -853,10 +850,6 @@ static void task_done(h2_mplx *m, h2_task *task, h2_req_engine *ngn) H2_STRM_MSG(stream, "task_done, stream open")); if (stream->input) { h2_beam_leave(stream->input); - h2_beam_mutex_disable(stream->input); - } - if (stream->output) { - h2_beam_mutex_disable(stream->output); } /* more data will not arrive, resume the stream */ @@ -869,10 +862,6 @@ static void task_done(h2_mplx *m, h2_task *task, h2_req_engine *ngn) H2_STRM_MSG(stream, "task_done, in hold")); if (stream->input) { h2_beam_leave(stream->input); - h2_beam_mutex_disable(stream->input); - } - if (stream->output) { - h2_beam_mutex_disable(stream->output); } stream_joined(m, stream); } @@ -969,29 +958,19 @@ static h2_stream *get_timed_out_busy_stream(h2_mplx *m) static apr_status_t unschedule_slow_tasks(h2_mplx *m) { h2_stream *stream; - int n, amax; + int n; /* Try to get rid of streams that occupy workers. Look for safe requests * that are repeatable. If none found, fail the connection. - * - * see: https://github.com/icing/mod_h2/issues/120 - * Enforcing m->limit_active (which can go as low as 2) was too - * aggressive for media streaming. Players needed to re-open streams/connections - * continously as they were reading large files very slowly. */ - amax = m->max_active/2; - if (m->limit_active > amax) { - amax = m->limit_active; - } - n = (m->tasks_active - amax - (int)h2_ihash_count(m->sredo)); - + n = (m->tasks_active - m->limit_active - (int)h2_ihash_count(m->sredo)); while (n > 0 && (stream = get_latest_repeatable_unsubmitted_stream(m))) { h2_task_rst(stream->task, H2_ERR_CANCEL); h2_ihash_add(m->sredo, stream); --n; } - if ((m->tasks_active - h2_ihash_count(m->sredo)) > amax) { + if ((m->tasks_active - h2_ihash_count(m->sredo)) > m->limit_active) { h2_stream *stream = get_timed_out_busy_stream(m); if (stream) { /* Too many busy workers, unable to cancel enough streams diff --git a/mod_http2/h2_task.c b/mod_http2/h2_task.c index 2ab41455..1f2f4c25 100644 --- a/mod_http2/h2_task.c +++ b/mod_http2/h2_task.c @@ -613,10 +613,6 @@ apr_status_t h2_task_do(h2_task *task, apr_thread_t *thread, int worker_id) h2_ctx_create_for(c, task); apr_table_setn(c->notes, H2_TASK_ID_NOTE, task->id); - if (task->input.beam) { - h2_beam_mutex_enable(task->input.beam); - } - h2_slave_run_pre_connection(c, ap_get_conn_socket(c)); task->input.bb = apr_brigade_create(task->pool, c->bucket_alloc); diff --git a/mod_http2/h2_util.c b/mod_http2/h2_util.c index e1879cb2..47dcdf56 100644 --- a/mod_http2/h2_util.c +++ b/mod_http2/h2_util.c @@ -112,6 +112,8 @@ void h2_util_camel_case_header(char *s, size_t len) } } +/* base64 url encoding ****************************************************************************/ + static const int BASE64URL_UINT6[] = { /* 0 1 2 3 4 5 6 7 8 9 a b c d e f */ -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, /* 0 */ @@ -174,6 +176,7 @@ apr_size_t h2_util_base64url_decode(const char **decoded, const char *encoded, n = ((BASE64URL_UINT6[ e[mlen+0] ] << 18) + (BASE64URL_UINT6[ e[mlen+1] ] << 12)); *d++ = n >> 16; + remain = 1; break; case 3: n = ((BASE64URL_UINT6[ e[mlen+0] ] << 18) + @@ -181,6 +184,7 @@ apr_size_t h2_util_base64url_decode(const char **decoded, const char *encoded, (BASE64URL_UINT6[ e[mlen+2] ] << 6)); *d++ = n >> 16; *d++ = n >> 8 & 0xffu; + remain = 2; break; default: /* do nothing */ break; @@ -189,78 +193,35 @@ apr_size_t h2_util_base64url_decode(const char **decoded, const char *encoded, } const char *h2_util_base64url_encode(const char *data, - apr_size_t len, apr_pool_t *pool) + apr_size_t dlen, apr_pool_t *pool) { - apr_size_t mlen = ((len+2)/3)*3; - apr_size_t slen = (mlen/3)*4; - apr_size_t i; + long i, len = (int)dlen; + apr_size_t slen = ((dlen+2)/3)*4 + 1; /* 0 terminated */ const unsigned char *udata = (const unsigned char*)data; - char *enc, *p = apr_pcalloc(pool, slen+1); /* 0 terminated */ + char *enc, *p = apr_pcalloc(pool, slen); enc = p; - for (i = 0; i < mlen; i+= 3) { + for (i = 0; i < len-2; i+= 3) { *p++ = BASE64URL_CHARS[ (udata[i] >> 2) & 0x3fu ]; - *p++ = BASE64URL_CHARS[ ((udata[i] << 4) + - ((i+1 < len)? (udata[i+1] >> 4) : 0)) & 0x3fu ]; - *p++ = BASE64URL_CHARS[ ((udata[i+1] << 2) + - ((i+2 < len)? (udata[i+2] >> 6) : 0)) & 0x3fu ]; - if (i+2 < len) { - *p++ = BASE64URL_CHARS[ udata[i+2] & 0x3fu ]; - } + *p++ = BASE64URL_CHARS[ ((udata[i] << 4) + (udata[i+1] >> 4)) & 0x3fu ]; + *p++ = BASE64URL_CHARS[ ((udata[i+1] << 2) + (udata[i+2] >> 6)) & 0x3fu ]; + *p++ = BASE64URL_CHARS[ udata[i+2] & 0x3fu ]; } - return enc; -} - -int h2_util_contains_token(apr_pool_t *pool, const char *s, const char *token) -{ - char *c; - if (s) { - if (!apr_strnatcasecmp(s, token)) { /* the simple life */ - return 1; - } - - for (c = ap_get_token(pool, &s, 0); c && *c; - c = *s? ap_get_token(pool, &s, 0) : NULL) { - if (!apr_strnatcasecmp(c, token)) { /* seeing the token? */ - return 1; - } - while (*s++ == ';') { /* skip parameters */ - ap_get_token(pool, &s, 0); - } - if (*s++ != ',') { /* need comma separation */ - return 0; - } + if (i < len) { + *p++ = BASE64URL_CHARS[ (udata[i] >> 2) & 0x3fu ]; + if (i == (len - 1)) { + *p++ = BASE64URL_CHARS[ (udata[i] << 4) & 0x3fu ]; } - } - return 0; -} - -const char *h2_util_first_token_match(apr_pool_t *pool, const char *s, - const char *tokens[], apr_size_t len) -{ - char *c; - apr_size_t i; - if (s && *s) { - for (c = ap_get_token(pool, &s, 0); c && *c; - c = *s? ap_get_token(pool, &s, 0) : NULL) { - for (i = 0; i < len; ++i) { - if (!apr_strnatcasecmp(c, tokens[i])) { - return tokens[i]; - } - } - while (*s++ == ';') { /* skip parameters */ - ap_get_token(pool, &s, 0); - } - if (*s++ != ',') { /* need comma separation */ - return 0; - } + else { + *p++ = BASE64URL_CHARS[ ((udata[i] << 4) + (udata[i+1] >> 4)) & 0x3fu ]; + *p++ = BASE64URL_CHARS[ (udata[i+1] << 2) & 0x3fu ]; } } - return NULL; + *p++ = '\0'; + return enc; } - /******************************************************************************* * ihash - hash for structs with int identifier ******************************************************************************/ diff --git a/mod_http2/h2_util.h b/mod_http2/h2_util.h index b2c25f69..01ccccd7 100644 --- a/mod_http2/h2_util.h +++ b/mod_http2/h2_util.h @@ -337,15 +337,6 @@ unsigned char h2_log2(int n); */ apr_size_t h2_util_table_bytes(apr_table_t *t, apr_size_t pair_extra); -/** - * Return != 0 iff the string s contains the token, as specified in - * HTTP header syntax, rfc7230. - */ -int h2_util_contains_token(apr_pool_t *pool, const char *s, const char *token); - -const char *h2_util_first_token_match(apr_pool_t *pool, const char *s, - const char *tokens[], apr_size_t len); - /** Match a header value against a string constance, case insensitive */ #define H2_HD_MATCH_LIT(l, name, nlen) \ ((nlen == sizeof(l) - 1) && !apr_strnatcasecmp(l, name)) diff --git a/mod_http2/h2_version.h b/mod_http2/h2_version.h index c3e64a2e..5826a789 100644 --- a/mod_http2/h2_version.h +++ b/mod_http2/h2_version.h @@ -26,7 +26,7 @@ * @macro * Version number of the http2 module as c string */ -#define MOD_HTTP2_VERSION "1.10.11-git" +#define MOD_HTTP2_VERSION "1.10.12-git" /** * @macro @@ -34,7 +34,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 0x010a0b +#define MOD_HTTP2_VERSION_NUM 0x010a0c #endif /* mod_h2_h2_version_h */ diff --git a/mod_http2/h2_workers.c b/mod_http2/h2_workers.c index 5abddd4a..12762506 100644 --- a/mod_http2/h2_workers.c +++ b/mod_http2/h2_workers.c @@ -98,6 +98,8 @@ static apr_status_t activate_slot(h2_workers *workers, h2_slot *slot) } } + 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,