Opened 5 years ago

Closed 5 years ago

#1797 closed defect (fixed)

grpc module handles WINDOW_UPDATE improperly on closed streams

Reported by: oleg.dropbox.com@… Owned by:
Priority: minor Milestone:
Component: other Version: 1.15.x
Keywords: grpc Cc:
uname -a: Linux 4.15.0-52-generic #56~16.04.1-Ubuntu SMP Thu Jun 6 12:03:31 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
nginx -V: nginx version: nginx/1.15.6
built by Bazel
built by gcc 4.9.4 (Ubuntu 4.9.4-2ubuntu1~12.04)
built with OpenSSL 1.1.0 (compatible; BoringSSL) (running with BoringSSL)
TLS SNI support enabled
configure arguments: --prefix=/etc/nginx --conf-path=/etc/nginx/nginx.conf --error-log-path=/var/log/nginx/error.log --pid-path=/var/run/nginx.pid --lock-path=/var/lock/nginx.lock --user=nginx --group=nginx --http-log-path=/var/log/nginx/access.log --http-client-body-temp-path=/var/lib/nginx/client_temp --http-fastcgi-temp-path=/var/lib/nginx/fastcgi_temp --http-proxy-temp-path=/var/lib/nginx/proxy_temp --http-scgi-temp-path=/var/lib/nginx/scgi_temp --http-uwsgi-temp-path=/var/lib/nginx/uwsgi_temp --with-compat --with-file-aio --with-threads --add-module=@ngx_brotli//:http_brotli_filter --add-module=@ngx_brotli//:http_brotli_static --add-module=@ngx_http_headers_more_filter//:http_headers_more_filter --add-module=@ngx_http_ip_tos_filter//:http_ip_tos_filter --add-module=@ngx_http_lua//:http_lua --add-module=dbx_dials_module --add-module=modules/nginx_upstream_check_module --add-module=modules/ngx_http_upstream_dynamic_module --add-module=ngx_http_streaming_filter --with-http_auth_request_module --with-http_gzip_static_module --with-http_realip_module --with-http_ssl_module --with-http_v2_module --with-openssl=boringssl --with-stream

Description

Current implementation fails the request on receiving WINDOW_UPDATE frame for closed upstream:

upstream sent frame for closed stream 1 while reading response header from upstream, client: 127.0.0.1, server: test, request: "POST /test.RequestIdEcho/StreamStream HTTP/2.0", upstream: "grpcs://127.0.0.1:5015", host: "test_user1"

But it shouldn't per the RFC: ​https://tools.ietf.org/html/rfc7540#section-5.1:

closed:
      The "closed" state is the terminal state.

      WINDOW_UPDATE or RST_STREAM frames can be received in this state
      for a short period after a DATA or HEADERS frame containing an
      END_STREAM flag is sent.  Until the remote peer receives and
      processes RST_STREAM or the frame bearing the END_STREAM flag, it
      might send frames of these types.  Endpoints MUST ignore
      WINDOW_UPDATE or RST_STREAM frames received in this state, though
      endpoints MAY choose to treat frames that arrive a significant
      time after sending END_STREAM as a connection error
      (Section 5.4.1) of type PROTOCOL_ERROR.

nginx with following patch passes the testcase:

diff --git a/src/http/modules/ngx_http_grpc_module.c b/src/http/modules/ngx_http_grpc_module.c
index e79c3224..d78409dc 100644
--- a/src/http/modules/ngx_http_grpc_module.c
+++ b/src/http/modules/ngx_http_grpc_module.c
@@ -1973,7 +1973,9 @@ ngx_http_grpc_filter(void *data, ssize_t bytes)
             }
 
             if (ctx->stream_id && ctx->done
-                && ctx->type != NGX_HTTP_V2_RST_STREAM_FRAME)
+                && ctx->type != NGX_HTTP_V2_RST_STREAM_FRAME
+                && ctx->type != NGX_HTTP_V2_WINDOW_UPDATE_FRAME)
+
             {
                 ngx_log_error(NGX_LOG_ERR, r->connection->log, 0,
                               "upstream sent frame for closed stream %ui",

Change History (3)

comment:1 by Sergey Kandaurov, 5 years ago

Yep, I considered this addition when cooking patch for ticket #1792.
It makes sense for me.

comment:2 by Maxim Dounin, 5 years ago

Keywords: grpc added

comment:3 by Ruslan Ermilov, 5 years ago

Resolution: → fixed
Status: new → closed

Fixed in 716eddd74bc2, thanks!

Note: See TracTickets for help on using tickets.