Yep, I've thought about this, but prefer not to modify buffer at all, since it feels more error-prone.
F.E: SB might decide to change number of dots, or reuse last header from this buffer, etc.

Do you feel strongly against printing "..." just at log line?
________________________________________
From: nginx-devel-bounces@nginx.org [nginx-devel-bounces@nginx.org] on behalf of Maxim Dounin [mdounin@mdounin.ru]
Sent: Tuesday, October 07, 2014 10:54 AM
To: nginx-devel@nginx.org
Subject: Re: Prevent buffer overrun on NGX_HTTP_REQUEST_HEADER_TOO_LARGE

Hello!

On Tue, Oct 07, 2014 at 05:23:57PM +0000, Bondarev, Daniil wrote:

> Hi,
>
> There is an issue in nginx, when it's returning NGX_HTTP_REQUEST_HEADER_TOO_LARGE
> in ngx_http_process_request_headers:
> When large header buffer is full and the last header size is 1 or 2 bytes more
> than NGX_MAX_ERROR_STR - 300, nginx will write 1 or 2 '.' symbols out of large
> header buffer, causing unpredictable behavior.
>
> To reproduce it you can send request with total headers size of 'large client buffers size' and
> the last header size of 1749. Valgrind will catch this issue:
>
> ==10776== Invalid write of size 1
> ==10776== at 0x426E84: ngx_http_process_request_headers (ngx_http_request.c:1230)
> ...
>
> The following patch fixes this issue:
>
> # HG changeset patch
> # User Daniil Bondarev <bondarev@amazon.com>
> # Date 1412401143 25200
> # Node ID 2bbb5284ca7ff658ad50254fe0c5bec14247ba75
> # Parent 6bbad2e732458bf53771e80c63a654b3d7f61963
> Prevent buffer overrun on NGX_HTTP_REQUEST_HEADER_TOO_LARGE
>
> When large header buffer is full and the last header size is 1 or 2 bytes more
> than NGX_MAX_ERROR_STR - 300, nginx will write 1 or 2 '.' symbols out of large
> header buffer, causing unpredictable behavior.
>
> The fix is, instead of modifying a buffer, just cut the header and print '...'
> in log line if header is too large.
>
> diff -r 6bbad2e73245 -r 2bbb5284ca7f src/http/ngx_http_request.c
> --- a/src/http/ngx_http_request.c Wed Aug 27 20:51:01 2014 +0400
> +++ b/src/http/ngx_http_request.c Fri Oct 03 22:39:03 2014 -0700
> @@ -1171,6 +1171,7 @@
> {
> u_char *p;
> size_t len;
> + size_t print_len;
> ssize_t n;
> ngx_int_t rc, rv;
> ngx_table_elt_t *h;
> @@ -1225,14 +1226,13 @@
>
> len = r->header_in->end - p;
>
> - if (len > NGX_MAX_ERROR_STR - 300) {
> - len = NGX_MAX_ERROR_STR - 300;
> - p[len++] = '.'; p[len++] = '.'; p[len++] = '.';
> - }
> + /* Since log line size is limited to NGX_MAX_ERROR_STR,
> + * nginx has to limit header size it will print into log. */
> + print_len = ngx_min(len, NGX_MAX_ERROR_STR - 300);
>
> ngx_log_error(NGX_LOG_INFO, c->log, 0,
> - "client sent too long header line: \"%*s\"",
> - len, r->header_name_start);
> + "client sent too long header line: \"%*s%s\"",
> + print_len, p, len != print_len ? "..." : "");

Thanks for the report.
What do you think about something as simple as:

--- a/src/http/ngx_http_request.c
+++ b/src/http/ngx_http_request.c
@@ -1226,7 +1226,7 @@ ngx_http_process_request_headers(ngx_eve
len = r->header_in->end - p;

if (len > NGX_MAX_ERROR_STR - 300) {
- len = NGX_MAX_ERROR_STR - 300;
+ len = NGX_MAX_ERROR_STR - 300 - 3;
p[len++] = '.'; p[len++] = '.'; p[len++] = '.';
}


?

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