Hello!
On Thu, Feb 16, 2017 at 05:00:20PM -0800, Daniil Bondarev wrote:
> # HG changeset patch
> # User Daniil Bondarev <bondarev@amazon.com>
> # 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 <mdounin@mdounin.ru>
# 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
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
