On Tuesday 13 June 2017 05:19:54 Piotr Sikora via nginx-devel wrote:
> # HG changeset patch
> # User Piotr Sikora <piotrsikora@google.com>
> # Date 1490351854 25200
> # Fri Mar 24 03:37:34 2017 -0700
> # Node ID 73f67e06ab103e0368d1810c6f8cac5c70c4e246
> # Parent 07a5d26b49f04425ff54cc998f885aa987b7823f
> HTTP/2: added support for trailers in HTTP responses.
>
> Signed-off-by: Piotr Sikora <piotrsikora@google.com>
>
[..]
> +
> + if (header[i].key.len > NGX_HTTP_V2_MAX_FIELD) {
> + ngx_log_error(NGX_LOG_WARN, r->connection->log, 0,
> + "too long response trailer name: \"%V\"",
> + &header[i].key);
> + return NULL;
> + }
> +
> + if (header[i].value.len > NGX_HTTP_V2_MAX_FIELD) {
> + ngx_log_error(NGX_LOG_WARN, r->connection->log, 0,
> + "too long response trailer value: \"%V: %V\"",
> + &header[i].key, &header[i].value);
> + return NULL;
> + }
[..]

I've overlooked this while doing previous review, but it looks strange.

Why do you use NGX_LOG_WARN for trailers headers? It results in
finalizing request with an error (in case of HTTP/2 it means RST_STREAM).

For main headers the NGX_LOG_CRIT level is used. It looks too serious,
but the WARN level is too low.

It seems the right log level for both cases is NGX_LOG_ERR.

So I'm going to commit the patch below:

# HG changeset patch
# User Valentin Bartenev <vbart@nginx.com>
# Date 1497453034 -10800
# Wed Jun 14 18:10:34 2017 +0300
# Node ID 460ed2f4e160bc6b0953456d1218381cdfc7e40b
# Parent 3c55863e6887ee764265fd43a878b3f793e0a7b9
HTTP/2: adjusted log level for too long response headers.

It's likely an upstream fault and the CRIT log level looks too high.

diff -r 3c55863e6887 -r 460ed2f4e160 src/http/v2/ngx_http_v2_filter_module.c
--- a/src/http/v2/ngx_http_v2_filter_module.c Tue Jun 13 17:01:08 2017 +0300
+++ b/src/http/v2/ngx_http_v2_filter_module.c Wed Jun 14 18:10:34 2017 +0300
@@ -385,14 +385,14 @@ ngx_http_v2_header_filter(ngx_http_reque
}

if (header[i].key.len > NGX_HTTP_V2_MAX_FIELD) {
- ngx_log_error(NGX_LOG_CRIT, fc->log, 0,
+ ngx_log_error(NGX_LOG_ERR, fc->log, 0,
"too long response header name: \"%V\"",
&header[i].key);
return NGX_ERROR;
}

if (header[i].value.len > NGX_HTTP_V2_MAX_FIELD) {
- ngx_log_error(NGX_LOG_CRIT, fc->log, 0,
+ ngx_log_error(NGX_LOG_ERR, fc->log, 0,
"too long response header value: \"%V: %V\"",
&header[i].key, &header[i].value);
return NGX_ERROR;

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