Hello, On Sat, Feb 18, 2017 at 6:10 PM, Maxim Dounin <[email protected]> wrote: > 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, >
Ah, good catch! > 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; > Sure, that works (: Thanks! > -- > Maxim Dounin > http://nginx.org/ > _______________________________________________ > nginx-devel mailing list > [email protected] > http://mailman.nginx.org/mailman/listinfo/nginx-devel _______________________________________________ nginx-devel mailing list [email protected] http://mailman.nginx.org/mailman/listinfo/nginx-devel
