Hello! On Thu, Feb 16, 2017 at 05:00:20PM -0800, Daniil Bondarev wrote:
> # HG changeset patch > # User Daniil Bondarev <[email protected]> > # Date 1485286710 28800 > # Tue Jan 24 11:38:30 2017 -0800 > # Node ID 8cd694e06443aaa1ed0601108681fa1c6f7297e0 > # Parent d84f48e571e449ee6c072a8d52cdea8e06b88ef7 > Always use default server configs for large buffers allocation > > Single http connection can flip between default server and virtual > servers: depending on parsed Host header for a current request nginx > changes current request configs. Currently this behavior might cause > buffer overrun while adding new buffer to hc->busy, if hc->busy was > allocated with config containing fewer large_client_header_buffers than > the current one. > > This change makes nginx to always use large_client_header_buffers from > http_connection config, which is default server config. > > diff -r d84f48e571e4 -r 8cd694e06443 src/http/ngx_http_request.c > --- a/src/http/ngx_http_request.c Tue Jan 24 17:02:19 2017 +0300 > +++ b/src/http/ngx_http_request.c Tue Jan 24 11:38:30 2017 -0800 > @@ -1447,7 +1447,9 @@ ngx_http_alloc_large_header_buffer(ngx_h > > old = request_line ? r->request_start : r->header_name_start; > > - cscf = ngx_http_get_module_srv_conf(r, ngx_http_core_module); > + hc = r->http_connection; > + > + cscf = ngx_http_get_module_srv_conf(hc->conf_ctx, ngx_http_core_module); > > if (r->state != 0 > && (size_t) (r->header_in->pos - old) > @@ -1456,8 +1458,6 @@ ngx_http_alloc_large_header_buffer(ngx_h > return NGX_DECLINED; > } > > - hc = r->http_connection; > - > if (hc->nfree) { > b = hc->free[--hc->nfree]; Thanks for spotting this. Another part of this problem is with hc->free. If a number of large header buffers configured in a virtual server is less than the one in the default server, saving buffers from hc->busy to hc->free for pipelined requests will also overflow allocated buffer. To fix this as well, the patch should be extend with something like this: @@ -2876,7 +2875,7 @@ ngx_http_set_keepalive(ngx_http_request_ * Now we would move the large header buffers to the free list. */ - cscf = ngx_http_get_module_srv_conf(r, ngx_http_core_module); + cscf = ngx_http_get_module_srv_conf(hc->conf_ctx, ngx_http_core_module); if (hc->free == NULL) { hc->free = ngx_palloc(c->pool, An alternative approach would be to convert hc->busy / hc->free to use chain links. This will preserve the possibility to have different large_client_header_buffers in different virtual hosts as long as the host is known before the limit is reached. Patch: # HG changeset patch # User Maxim Dounin <[email protected]> # Date 1487467695 -10800 # Sun Feb 19 04:28:15 2017 +0300 # Node ID 3412c0c019f0f24432bac8b3e65ec03ba69073cb # Parent 87cf6ddb41c216876d13cffa5e637a61b159362c Converted hc->busy/hc->free to use chain links. Most notably, this fixes possible buffer overflows if number of buffers in a virtual server is different from the one in the default server. Reported by Daniil Bondarev. diff --git a/src/http/ngx_http_request.c b/src/http/ngx_http_request.c --- a/src/http/ngx_http_request.c +++ b/src/http/ngx_http_request.c @@ -549,7 +549,7 @@ ngx_http_create_request(ngx_connection_t ngx_set_connection_log(r->connection, clcf->error_log); - r->header_in = hc->nbusy ? hc->busy[0] : c->buffer; + r->header_in = hc->busy ? hc->busy->buf : c->buffer; if (ngx_list_init(&r->headers_out.headers, r->pool, 20, sizeof(ngx_table_elt_t)) @@ -1431,6 +1431,7 @@ ngx_http_alloc_large_header_buffer(ngx_h { u_char *old, *new; ngx_buf_t *b; + ngx_chain_t *cl; ngx_http_connection_t *hc; ngx_http_core_srv_conf_t *cscf; @@ -1460,8 +1461,11 @@ ngx_http_alloc_large_header_buffer(ngx_h hc = r->http_connection; - if (hc->nfree) { - b = hc->free[--hc->nfree]; + if (hc->free) { + cl = hc->free; + hc->free = cl->next; + + b = cl->buf; ngx_log_debug2(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, "http large header free: %p %uz", @@ -1469,20 +1473,19 @@ ngx_http_alloc_large_header_buffer(ngx_h } else if (hc->nbusy < cscf->large_client_header_buffers.num) { - if (hc->busy == NULL) { - hc->busy = ngx_palloc(r->connection->pool, - cscf->large_client_header_buffers.num * sizeof(ngx_buf_t *)); - if (hc->busy == NULL) { - return NGX_ERROR; - } - } - b = ngx_create_temp_buf(r->connection->pool, cscf->large_client_header_buffers.size); if (b == NULL) { return NGX_ERROR; } + cl = ngx_alloc_chain_link(r->connection->pool); + if (cl == NULL) { + return NGX_ERROR; + } + + cl->buf = b; + ngx_log_debug2(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, "http large header alloc: %p %uz", b->pos, b->end - b->last); @@ -1491,7 +1494,9 @@ ngx_http_alloc_large_header_buffer(ngx_h return NGX_DECLINED; } - hc->busy[hc->nbusy++] = b; + cl->next = hc->busy; + hc->busy = cl; + hc->nbusy++; if (r->state == 0) { /* @@ -2835,12 +2840,11 @@ static void ngx_http_set_keepalive(ngx_http_request_t *r) { int tcp_nodelay; - ngx_int_t i; ngx_buf_t *b, *f; + ngx_chain_t *cl, *ln; ngx_event_t *rev, *wev; ngx_connection_t *c; ngx_http_connection_t *hc; - ngx_http_core_srv_conf_t *cscf; ngx_http_core_loc_conf_t *clcf; c = r->connection; @@ -2876,27 +2880,15 @@ ngx_http_set_keepalive(ngx_http_request_ * Now we would move the large header buffers to the free list. */ - cscf = ngx_http_get_module_srv_conf(r, ngx_http_core_module); - - if (hc->free == NULL) { - hc->free = ngx_palloc(c->pool, - cscf->large_client_header_buffers.num * sizeof(ngx_buf_t *)); - - if (hc->free == NULL) { - ngx_http_close_request(r, 0); - return; - } - } - - for (i = 0; i < hc->nbusy - 1; i++) { - f = hc->busy[i]; - hc->free[hc->nfree++] = f; + for (cl = hc->busy; cl->next; cl = cl->next) { + f = cl->next->buf; f->pos = f->start; f->last = f->start; } - hc->busy[0] = b; - hc->nbusy = 1; + cl->next = hc->free; + hc->free = hc->busy->next; + hc->busy->next = NULL; } } @@ -2966,28 +2958,24 @@ ngx_http_set_keepalive(ngx_http_request_ b->last = b->start; } - ngx_log_debug2(NGX_LOG_DEBUG_HTTP, c->log, 0, "hc free: %p %i", - hc->free, hc->nfree); - - if (hc->free) { - for (i = 0; i < hc->nfree; i++) { - ngx_pfree(c->pool, hc->free[i]->start); - hc->free[i] = NULL; - } - - hc->nfree = 0; + ngx_log_debug1(NGX_LOG_DEBUG_HTTP, c->log, 0, "hc free: %p", + hc->free); + + for (cl = hc->free; cl; /* void */) { + ln = cl; + cl = cl->next; + ngx_pfree(c->pool, ln->buf->start); + ngx_free_chain(r->pool, ln); } ngx_log_debug2(NGX_LOG_DEBUG_HTTP, c->log, 0, "hc busy: %p %i", hc->busy, hc->nbusy); - if (hc->busy) { - for (i = 0; i < hc->nbusy; i++) { - ngx_pfree(c->pool, hc->busy[i]->start); - hc->busy[i] = NULL; - } - - hc->nbusy = 0; + for (cl = hc->busy; cl; /* void */) { + ln = cl; + cl = cl->next; + ngx_pfree(c->pool, ln->buf->start); + ngx_free_chain(r->pool, ln); } #if (NGX_HTTP_SSL) diff --git a/src/http/ngx_http_request.h b/src/http/ngx_http_request.h --- a/src/http/ngx_http_request.h +++ b/src/http/ngx_http_request.h @@ -309,11 +309,10 @@ typedef struct { #endif #endif - ngx_buf_t **busy; + ngx_chain_t *busy; ngx_int_t nbusy; - ngx_buf_t **free; - ngx_int_t nfree; + ngx_chain_t *free; unsigned ssl:1; unsigned proxy_protocol:1; -- Maxim Dounin http://nginx.org/ _______________________________________________ nginx-devel mailing list [email protected] http://mailman.nginx.org/mailman/listinfo/nginx-devel
