On Sunday 26 March 2017 01:41:11 Piotr Sikora via nginx-devel wrote:
> # HG changeset patch
> # User Piotr Sikora <piotrsik...@google.com>
> # Date 1490516704 25200
> # Sun Mar 26 01:25:04 2017 -0700
> # Node ID 899a53d2789b8c6bafdd5e40d78b4e92dd32dd10
> # Parent 22be63bf21edaa1b8ea916c7d8cd4e5fe4892061
> HTTP/2: fix flow control with padded DATA frames.
>
> Previously, flow control didn't account for padding in DATA frames,
> which meant that its view of the world could drift from peer's view
> by up to 256 bytes per received padded DATA frame, which could lead
> to a deadlock.
>
> Signed-off-by: Piotr Sikora <piotrsik...@google.com>
>
> diff -r 22be63bf21ed -r 899a53d2789b src/http/v2/ngx_http_v2.c
> --- a/src/http/v2/ngx_http_v2.c
> +++ b/src/http/v2/ngx_http_v2.c
> @@ -783,9 +783,12 @@ ngx_http_v2_state_head(ngx_http_v2_conne
> static u_char *
> ngx_http_v2_state_data(ngx_http_v2_connection_t *h2c, u_char *pos, u_char
> *end)
> {
> + size_t size;
> ngx_http_v2_node_t *node;
> ngx_http_v2_stream_t *stream;
>
> + size = h2c->state.length;
> +
> if (h2c->state.flags & NGX_HTTP_V2_PADDED_FLAG) {
>
> if (h2c->state.length == 0) {
> @@ -802,33 +805,32 @@ ngx_http_v2_state_data(ngx_http_v2_conne
> }
>
> h2c->state.padding = *pos++;
> - h2c->state.length--;
> -
> - if (h2c->state.padding > h2c->state.length) {
> +
> + if (h2c->state.padding >= size) {
> ngx_log_error(NGX_LOG_INFO, h2c->connection->log, 0,
> "client sent padded DATA frame "
> "with incorrect length: %uz, padding: %uz",
> - h2c->state.length, h2c->state.padding);
> + size, h2c->state.padding);
>
> return ngx_http_v2_connection_error(h2c, NGX_HTTP_V2_SIZE_ERROR);
> }
>
> - h2c->state.length -= h2c->state.padding;
> + h2c->state.length -= 1 + h2c->state.padding;
> }
IMHO, the previous version of this fragment with explicit h2c->state.length
decrement right after reading the padding size and "> h2c->state.length"
condition is more readable.
YMMV.
>
> ngx_log_debug0(NGX_LOG_DEBUG_HTTP, h2c->connection->log, 0,
> "http2 DATA frame");
>
> - if (h2c->state.length > h2c->recv_window) {
> + if (size > h2c->recv_window) {
> ngx_log_error(NGX_LOG_INFO, h2c->connection->log, 0,
> "client violated connection flow control: "
> "received DATA frame length %uz, available window %uz",
> - h2c->state.length, h2c->recv_window);
> + size, h2c->recv_window);
>
> return ngx_http_v2_connection_error(h2c,
> NGX_HTTP_V2_FLOW_CTRL_ERROR);
> }
>
> - h2c->recv_window -= h2c->state.length;
> + h2c->recv_window -= size;
>
> if (h2c->recv_window < NGX_HTTP_V2_MAX_WINDOW / 4) {
>
> @@ -854,11 +856,11 @@ ngx_http_v2_state_data(ngx_http_v2_conne
>
> stream = node->stream;
>
> - if (h2c->state.length > stream->recv_window) {
> + if (size > stream->recv_window) {
> ngx_log_error(NGX_LOG_INFO, h2c->connection->log, 0,
> "client violated flow control for stream %ui: "
> "received DATA frame length %uz, available window %uz",
> - node->id, h2c->state.length, stream->recv_window);
> + node->id, size, stream->recv_window);
>
> if (ngx_http_v2_terminate_stream(h2c, stream,
> NGX_HTTP_V2_FLOW_CTRL_ERROR)
> @@ -871,7 +873,7 @@ ngx_http_v2_state_data(ngx_http_v2_conne
> return ngx_http_v2_state_skip_padded(h2c, pos, end);
> }
>
> - stream->recv_window -= h2c->state.length;
> + stream->recv_window -= size;
>
> if (stream->no_flow_control
> && stream->recv_window < NGX_HTTP_V2_MAX_WINDOW / 4)
>
Everything else looks good to me.
wbr, Valentin V. Bartenev
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
