Welcome! Log In Create A New Profile

Advanced

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

geniuss99
March 17, 2021 11:28AM
> 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.

Feel free to modify the patch as you need. I merely fixed a bug at the
place of origin.



> (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.)

Yeah, I know. I just wanted to make the code clear and compact so that
you understand everything.
Maybe I shouldn't have named it a patch. You may consider it a
pseudocode then :)



> 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?

I use gRPC module to pass normal http requests to h2 upstreams, not gRPC ones.
Works like a charm.

So can't give you any gRPC servers for testing. You can observe the bug by
connecting to developers.google.com h2 server for example. They use padding
in DATA streams.

On Wed, Mar 17, 2021 at 2:45 AM Maxim Dounin <mdounin@mdounin.ru> wrote:
>
> 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
_______________________________________________
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 165 March 16, 2021 03:06PM

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

Maxim Dounin 19 March 16, 2021 07:46PM

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

geniuss99 15 March 17, 2021 11:28AM

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

Maxim Dounin 19 March 17, 2021 08:56PM

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

geniuss99 16 March 19, 2021 12:34PM

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

Maxim Dounin 35 March 23, 2021 11:14AM

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

Sergey Kandaurov 16 March 23, 2021 08:26AM



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

Online Users

Guests: 58
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