Maxim Dounin
March 16, 2021 07:46PM
Hello!

On Tue, Mar 16, 2021 at 10:04:27PM +0300, geniuss99 wrote:

> src/http/modules/ngx_http_grpc_module.c | 16 +++++++++++++---
> 1 files changed, 13 insertions(+), 3 deletions(-)
>
>
> # HG changeset patch
> # User geniuss99 <geniuss.dev@gmail.com>
> # Date 1615921026 -10800
> # Tue Mar 16 21:57:06 2021 +0300
> # Node ID 13552d5b785104f9d137d956a6cbef25ec09b345
> # Parent ed1348e8e25381b3b1a2540289effcf7ccec6fd6
> gRPC: fixed bug when padding is used in DATA frame.
>
> As per RFC 7540 DATA frame MAY contain padding if the PADDED flag is set
> in "Type" field (see clause 6.1).
>
> When such frame is sent from an upstream which previously defined payload
> length via "Content-Length" header nginx fails with an error: "upstream sent
> response body larger than indicated content length".
>
> This happens because padding is not taken into account while comparing expected
> and received payload length.

Thanks for reporting this, looks like a bug introduced by me in
7680:39501ce97e29 (nginx 1.19.1).

Do you have any particular servers/gRPC libraries in mind which
actually use padding on DATA frames, and so nginx cannot talk with
them after introduction of the check in question?

>
> diff -r ed1348e8e253 -r 13552d5b7851 src/http/modules/ngx_http_grpc_module.c
> --- a/src/http/modules/ngx_http_grpc_module.c Thu Mar 11 20:49:39 2021 +0300
> +++ b/src/http/modules/ngx_http_grpc_module.c Tue Mar 16 21:57:06 2021 +0300
> @@ -2075,14 +2075,24 @@
> }
>
> if (ctx->length != -1) {
> - if ((off_t) ctx->rest > ctx->length) {
> + if (ctx->flags & NGX_HTTP_V2_PADDED_FLAG) {
> + if (b->pos < b->last) {
> + u_char pad_length = *b->pos;
> + size_t payload_data_length = ctx->rest - pad_length - 1; // frame_payload_size_bytes - pad_length_bytes - pad_length_field_1_byte
> +
> + ctx->length -= payload_data_length;
> + }
> +
> + } else {
> + ctx->length -= ctx->rest;
> + }
> +
> + if (ctx->length < 0) {
> ngx_log_error(NGX_LOG_ERR, r->connection->log, 0,
> "upstream sent response body larger "
> "than indicated content length");
> return NGX_ERROR;
> }
> -
> - ctx->length -= ctx->rest;
> }
>
> if (ctx->rest > ctx->recv_window) {

The fix looks suboptimal. Rather, the check should be moved to
the end of the function, where padding is already handled. I'll
take a look how to fix this properly.

(Note well that as per nginx style, local variables are defined at
the start of the function, "//" comments are not used, and maximum
line length is 80 characters. The "Contributing Changes"
article[1] have additional hints and links to appropriate part of
the development guide with the details.)

[1] http://nginx.org/en/docs/contributing_changes.html

--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
Subject Author Views Posted

[PATCH] gRPC: fixed bug when padding is used in DATA frame

geniuss99 196 March 16, 2021 03:06PM

Re: [PATCH] gRPC: fixed bug when padding is used in DATA frame

Maxim Dounin 31 March 16, 2021 07:46PM

Re: [PATCH] gRPC: fixed bug when padding is used in DATA frame

geniuss99 29 March 17, 2021 11:28AM

Re: [PATCH] gRPC: fixed bug when padding is used in DATA frame

Maxim Dounin 31 March 17, 2021 08:56PM

Re: [PATCH] gRPC: fixed bug when padding is used in DATA frame

geniuss99 29 March 19, 2021 12:34PM

Re: [PATCH] gRPC: fixed bug when padding is used in DATA frame

Maxim Dounin 50 March 23, 2021 11:14AM

Re: [PATCH] gRPC: fixed bug when padding is used in DATA frame

Sergey Kandaurov 31 March 23, 2021 08:26AM



Sorry, you do not have permission to post/reply in this forum.

Online Users

Guests: 67
Record Number of Users: 6 on February 13, 2018
Record Number of Guests: 421 on December 02, 2018
Powered by nginx      Powered by FreeBSD      PHP Powered      Powered by MariaDB      ipv6 ready