[PATCH] HTTP/2: emit PROTOCOL_ERROR on invalid WINDOW_UPDATE increments

[Valentin V. Bartenev]

On Sunday 26 March 2017 01:41:15 Piotr Sikora via nginx-devel wrote:
> # HG changeset patch
> # User Piotr Sikora <piotrsikora at google.com>
> # Date 1490516706 25200
> #      Sun Mar 26 01:25:06 2017 -0700
> # Node ID 9bbcacbdf6bd858a34a9dfd1ac2185eb8fc8c82f
> # Parent  22be63bf21edaa1b8ea916c7d8cd4e5fe4892061
> HTTP/2: emit PROTOCOL_ERROR on invalid WINDOW_UPDATE increments.
> 
> Signed-off-by: Piotr Sikora <piotrsikora at google.com>
> 
> diff -r 22be63bf21ed -r 9bbcacbdf6bd src/http/v2/ngx_http_v2.c
> --- a/src/http/v2/ngx_http_v2.c
> +++ b/src/http/v2/ngx_http_v2.c
> @@ -2173,6 +2173,22 @@ ngx_http_v2_state_window_update(ngx_http
>  
>          stream = node->stream;
>  
> +        if (window == 0) {
> +            ngx_log_error(NGX_LOG_INFO, h2c->connection->log, 0,
> +                          "client sent WINDOW_UPDATE frame for stream %ui "
> +                          "with incorrect window increment 0", h2c->state.sid);
> +
> +            if (ngx_http_v2_terminate_stream(h2c, stream,
> +                                             NGX_HTTP_V2_PROTOCOL_ERROR)
> +                == NGX_ERROR)
> +            {
> +                return ngx_http_v2_connection_error(h2c,
> +                                                    NGX_HTTP_V2_INTERNAL_ERROR);
> +            }
> +
> +            return ngx_http_v2_state_complete(h2c, pos, end);
> +        }
> +
>          if (window > (size_t) (NGX_HTTP_V2_MAX_WINDOW - stream->send_window)) {
>  
>              ngx_log_error(NGX_LOG_INFO, h2c->connection->log, 0,
> @@ -2211,6 +2227,14 @@ ngx_http_v2_state_window_update(ngx_http
>          return ngx_http_v2_state_complete(h2c, pos, end);
>      }
>  
> +    if (window == 0) {
> +        ngx_log_error(NGX_LOG_INFO, h2c->connection->log, 0,
> +                      "client sent WINDOW_UPDATE frame "
> +                      "with incorrect window increment 0");
> +
> +        return ngx_http_v2_connection_error(h2c, NGX_HTTP_V2_PROTOCOL_ERROR);
> +    }
> +
>      if (window > NGX_HTTP_V2_MAX_WINDOW - h2c->send_window) {
>          ngx_log_error(NGX_LOG_INFO, h2c->connection->log, 0,
>                        "client violated connection flow control: "


I'm not sure that strictly following RFC here is worth the effort.

It seems there's no other "harm" from zero window updates except that it
allows to reset timers without any progress.  That's only slightly worse
than 1-bytes window updates.

The downside is additional code and intolerance to potential client bugs.

Also note that in your implementation if zero window update is received
for unknown stream then it's silently ignored.

  wbr, Valentin V. Bartenev