Hello,

On Sat, Feb 18, 2017 at 6:10 PM, Maxim Dounin <mdounin@mdounin.ru> wrote:
> 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,
>

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 <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;
>

Sure, that works (: Thanks!

> --
> Maxim Dounin
> http://nginx.org/
> _______________________________________________
> nginx-devel mailing list
> nginx-devel@nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel